Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(C++): Support underlying_type for union #7954

Merged
merged 4 commits into from
May 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,7 @@ if(FLATBUFFERS_BUILD_TESTS)
compile_schema_for_test(tests/64bit/test_64bit.fbs "${FLATC_OPT_COMP};--bfbs-gen-embed")
compile_schema_for_test(tests/64bit/evolution/v1.fbs "${FLATC_OPT_COMP}")
compile_schema_for_test(tests/64bit/evolution/v2.fbs "${FLATC_OPT_COMP}")
compile_schema_for_test(tests/union_underlying_type_test.fbs "${FLATC_OPT_COMP}")

if(FLATBUFFERS_CODE_SANITIZE)
add_fsanitize_to_target(flattests ${FLATBUFFERS_CODE_SANITIZE})
Expand Down
1 change: 1 addition & 0 deletions include/flatbuffers/idl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1151,6 +1151,7 @@ class Parser : public ParserState {
bool SupportsOptionalScalars() const;
bool SupportsDefaultVectorsAndStrings() const;
bool Supports64BitOffsets() const;
bool SupportsUnionUnderlyingType() const;
Namespace *UniqueNamespace(Namespace *ns);

FLATBUFFERS_CHECKED_ERROR RecurseError();
Expand Down
14 changes: 10 additions & 4 deletions src/idl_gen_cpp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,12 @@ class CppGenerator : public BaseGenerator {
if (type.enum_def) return WrapInNameSpace(*type.enum_def);
if (type.base_type == BASE_TYPE_BOOL) return "bool";
}
return StringOf(type.base_type);
// Get real underlying type for union type
auto base_type = type.base_type;
if (type.base_type == BASE_TYPE_UTYPE && type.enum_def != nullptr) {
base_type = type.enum_def->underlying_type.base_type;
}
return StringOf(base_type);
}

// Return a C++ pointer type, specialized to the actual struct/table types,
Expand Down Expand Up @@ -1048,7 +1053,7 @@ class CppGenerator : public BaseGenerator {

std::string UnionVectorVerifySignature(const EnumDef &enum_def) {
const std::string name = Name(enum_def);
const std::string &type = opts_.scoped_enums ? name : "uint8_t";
const std::string &type = opts_.scoped_enums ? name : GenTypeBasic(enum_def.underlying_type, false);
return "bool Verify" + name + "Vector" +
"(::flatbuffers::Verifier &verifier, " +
"const ::flatbuffers::Vector<::flatbuffers::Offset<void>> "
Expand Down Expand Up @@ -3496,12 +3501,13 @@ class CppGenerator : public BaseGenerator {
}
case BASE_TYPE_UTYPE: {
value = StripUnionType(value);
auto underlying_type = GenTypeBasic(vector_type, false);
const std::string &type = opts_.scoped_enums
? Name(*field.value.type.enum_def)
: "uint8_t";
: underlying_type;
auto enum_value = "__va->_" + value + "[i].type";
if (!opts_.scoped_enums)
enum_value = "static_cast<uint8_t>(" + enum_value + ")";
enum_value = "static_cast<" + underlying_type + ">(" + enum_value + ")";

code += "_fbb.CreateVector<" + type + ">(" + value +
".size(), [](size_t i, _VectorArgs *__va) { return " +
Expand Down
54 changes: 41 additions & 13 deletions src/idl_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -947,8 +947,12 @@ CheckedError Parser::ParseField(StructDef &struct_def) {
if (type.base_type == BASE_TYPE_UNION) {
// For union fields, add a second auto-generated field to hold the type,
// with a special suffix.
ECHECK(AddField(struct_def, name + UnionTypeFieldSuffix(),
type.enum_def->underlying_type, &typefield));

// To ensure compatibility with many codes that rely on the BASE_TYPE_UTYPE value to identify union type fields.
Type union_type(type.enum_def->underlying_type);
union_type.base_type = BASE_TYPE_UTYPE;
ECHECK(AddField(struct_def, name + UnionTypeFieldSuffix(),union_type, &typefield));

} else if (IsVector(type) && type.element == BASE_TYPE_UNION) {
advanced_features_ |= reflection::AdvancedUnionFeatures;
// Only cpp, js and ts supports the union vector feature so far.
Expand Down Expand Up @@ -2482,23 +2486,39 @@ CheckedError Parser::ParseEnum(const bool is_union, EnumDef **dest,
&GetPooledString(RelativeToRootPath(opts.project_root, filename));
}
enum_def->doc_comment = enum_comment;
if (!is_union && !opts.proto_mode) {
if (!opts.proto_mode) {
// Give specialized error message, since this type spec used to
// be optional in the first FlatBuffers release.
bool explicit_underlying_type = false;
if (!Is(':')) {
return Error(
"must specify the underlying integer type for this"
" enum (e.g. \': short\', which was the default).");
// Enum is forced to have an explicit underlying type in declaration.
if (!is_union) {
return Error(
sssooonnnggg marked this conversation as resolved.
Show resolved Hide resolved
"must specify the underlying integer type for this"
" enum (e.g. \': short\', which was the default).");
}
} else {
// Union underlying type is only supported for cpp
if (is_union && !SupportsUnionUnderlyingType()) {
return Error(
"Underlying type for union is not yet supported in at least one of "
"the specified programming languages.");
}
NEXT();
explicit_underlying_type = true;
}
// Specify the integer type underlying this enum.
ECHECK(ParseType(enum_def->underlying_type));
if (!IsInteger(enum_def->underlying_type.base_type) ||
IsBool(enum_def->underlying_type.base_type))
return Error("underlying enum type must be integral");
// Make this type refer back to the enum it was derived from.
enum_def->underlying_type.enum_def = enum_def;

if (explicit_underlying_type) {
// Specify the integer type underlying this enum.
ECHECK(ParseType(enum_def->underlying_type));
if (!IsInteger(enum_def->underlying_type.base_type) || IsBool(enum_def->underlying_type.base_type)) {
return Error("underlying " + std::string(is_union ? "union" : "enum") + "type must be integral");
}

// Make this type refer back to the enum it was derived from.
enum_def->underlying_type.enum_def = enum_def;
}

}
ECHECK(ParseMetaData(&enum_def->attributes));
const auto underlying_type = enum_def->underlying_type.base_type;
Expand Down Expand Up @@ -2697,6 +2717,10 @@ bool Parser::Supports64BitOffsets() const {
~(IDLOptions::kCpp | IDLOptions::kJson | IDLOptions::kBinary)) == 0;
}

bool Parser::SupportsUnionUnderlyingType() const {
return (opts.lang_to_generate & ~IDLOptions::kCpp) == 0;
}

Namespace *Parser::UniqueNamespace(Namespace *ns) {
for (auto it = namespaces_.begin(); it != namespaces_.end(); ++it) {
if (ns->components == (*it)->components) {
Expand Down Expand Up @@ -4428,6 +4452,10 @@ std::string Parser::ConformTo(const Parser &base) {
return "values differ for enum: " + enum_val.name;
}
}
// Check underlying type changes
if (enum_def_base->underlying_type.base_type != enum_def.underlying_type.base_type) {
return "underlying type differ for " + std::string(enum_def.is_union ? "union: " : "enum: ") + qualified_name;
}
}
return "";
}
Expand Down
1 change: 1 addition & 0 deletions tests/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ cc_test(
"test_assert.h",
"test_builder.cpp",
"test_builder.h",
"union_underlying_type_test_generated.h",
"union_vector/union_vector_generated.h",
"util_test.cpp",
"util_test.h",
Expand Down
7 changes: 7 additions & 0 deletions tests/evolution_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ void ConformTest() {
test_conform(ref2, "enum E:int32 { A } table T2 { df:byte; f:E; }",
"field renamed to different type: T2.df (renamed from T2.f)");

// Check enum underlying type changes.
test_conform("enum E:int32 {A}", "enum E: byte {A}", "underlying type differ for enum: E");

// Check union underlying type changes.
const char ref3[] = "table A {} table B {} union C {A, B}";
test_conform(ref3, "table A {} table B {} union C:int32 {A, B}", "underlying type differ for union: C");

// Check conformity for Offset64-related changes.
{
const char ref[] = "table T { a:[uint8]; b:string; }";
Expand Down
9 changes: 9 additions & 0 deletions tests/parser_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,15 @@ void ParseUnionTest() {
"table B { e:U; } root_type B;"
"{ e_type: N_A, e: {} }"),
true);

// Test union underlying type
const char *source = "table A {} table B {} union U : int {A, B} table C {test_union: U; test_vector_of_union: [U];}";
flatbuffers::Parser parser3;
parser3.opts.lang_to_generate = flatbuffers::IDLOptions::kCpp;
TEST_EQ(parser3.Parse(source), true);

parser3.opts.lang_to_generate &= flatbuffers::IDLOptions::kJava;
TEST_EQ(parser3.Parse(source), false);
}

void ValidSameNameDifferentNamespaceTest() {
Expand Down
37 changes: 37 additions & 0 deletions tests/test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "proto_test.h"
#include "reflection_test.h"
#include "union_vector/union_vector_generated.h"
#include "union_underlying_type_test_generated.h"
#if !defined(_MSC_VER) || _MSC_VER >= 1700
# include "arrays_test_generated.h"
#endif
Expand Down Expand Up @@ -1540,6 +1541,41 @@ void DoNotRequireEofTest(const std::string &tests_data_path) {
}
#endif

void UnionUnderlyingTypeTest() {
using namespace UnionUnderlyingType;
TEST_ASSERT(sizeof(ABC) == sizeof(uint32_t));
TEST_ASSERT(ABC::ABC_A == 555);
TEST_ASSERT(ABC::ABC_B == 666);
TEST_ASSERT(ABC::ABC_C == 777);

DT buffer;
AT a;
a.a = 42;
BT b;
b.b = "foo";
CT c;
c.c = true;
buffer.test_union = ABCUnion();
buffer.test_union.Set(a);
buffer.test_vector_of_union.resize(3);
buffer.test_vector_of_union[0].Set(a);
buffer.test_vector_of_union[1].Set(b);
buffer.test_vector_of_union[2].Set(c);

flatbuffers::FlatBufferBuilder fbb;
auto offset = D::Pack(fbb, &buffer);
fbb.Finish(offset);

auto *root =
flatbuffers::GetRoot<D>(fbb.GetBufferPointer());
DT unpacked;
root->UnPackTo(&unpacked);

TEST_ASSERT(unpacked.test_union == buffer.test_union);
TEST_ASSERT(unpacked.test_vector_of_union == buffer.test_vector_of_union);

}

static void Offset64Tests() {
Offset64Test();
Offset64SerializedFirst();
Expand Down Expand Up @@ -1663,6 +1699,7 @@ int FlatBufferTests(const std::string &tests_data_path) {
FixedSizedStructArrayKeyInStructTest();
EmbeddedSchemaAccess();
Offset64Tests();
UnionUnderlyingTypeTest();
return 0;
}
} // namespace
Expand Down
17 changes: 17 additions & 0 deletions tests/union_underlying_type_test.fbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@

namespace UnionUnderlyingType;

table A {
a: int;
}
table B {
b: string;
}
table C {
c: bool;
}
union ABC: int { A = 555, B = 666, C = 777}
sssooonnnggg marked this conversation as resolved.
Show resolved Hide resolved
table D {
test_union: ABC;
test_vector_of_union: [ABC];
}
Loading
Loading