From c66683f27fbb4a6af70d3ff708e2c7f37cee0c60 Mon Sep 17 00:00:00 2001 From: Wouter van Oortmerssen Date: Mon, 19 Dec 2016 15:21:08 -0800 Subject: [PATCH] Add ::Set function to Unions to make memory ownership clear. Unions own the NativeTable* value member because they need to destroy them when the Union goes out of scope. Currently, the data is destroyed by calling delete, which means that the member needs to be allocated with new. However, making the allocation the responsibility of the client and the destruction the responsibility of the Union can lead to potential errors. Adding a Set function will ensure that the memory is allocated correctly so that it can be deleted later. From cl/142161569. Change-Id: I4605f26d2749164819bfae0140e5fae08442b50a --- samples/monster_generated.h | 46 +++++++++++------- src/idl_gen_cpp.cpp | 85 +++++++++++++++++++++------------- tests/monster_test_generated.h | 50 ++++++++++++-------- 3 files changed, 112 insertions(+), 69 deletions(-) diff --git a/samples/monster_generated.h b/samples/monster_generated.h index 1833c62d5..2504e8304 100644 --- a/samples/monster_generated.h +++ b/samples/monster_generated.h @@ -38,21 +38,6 @@ enum Equipment { Equipment_MAX = Equipment_Weapon }; -struct EquipmentUnion { - Equipment type; - - flatbuffers::NativeTable *table; - EquipmentUnion() : type(Equipment_NONE), table(nullptr) {} - EquipmentUnion(const EquipmentUnion &); - EquipmentUnion &operator=(const EquipmentUnion &); - ~EquipmentUnion(); - - static flatbuffers::NativeTable *UnPack(const void *union_obj, Equipment type, const flatbuffers::resolver_function_t *resolver); - flatbuffers::Offset Pack(flatbuffers::FlatBufferBuilder &_fbb, const flatbuffers::rehasher_function_t *rehasher = nullptr) const; - - WeaponT *AsWeapon() { return type == Equipment_Weapon ? reinterpret_cast(table) : nullptr; } -}; - inline const char **EnumNamesEquipment() { static const char *names[] = { "NONE", "Weapon", nullptr }; return names; @@ -68,6 +53,31 @@ template<> struct EquipmentTraits { static const Equipment enum_value = Equipment_Weapon; }; +struct EquipmentUnion { + Equipment type = Equipment_NONE; + + flatbuffers::NativeTable *table = nullptr; + EquipmentUnion() : type(Equipment_NONE), table(nullptr) {} + EquipmentUnion(const EquipmentUnion &); + EquipmentUnion &operator=(const EquipmentUnion &); + ~EquipmentUnion() { Reset(); } + void Reset(); + + template + void Set(T&& value) { + Reset(); + type = EquipmentTraits::enum_value; + if (type != Equipment_NONE) { + table = new T(std::move(value)); + } + } + + static flatbuffers::NativeTable *UnPack(const void *union_obj, Equipment type, const flatbuffers::resolver_function_t *resolver); + flatbuffers::Offset Pack(flatbuffers::FlatBufferBuilder &_fbb, const flatbuffers::rehasher_function_t *rehasher = nullptr) const; + + WeaponT *AsWeapon() { return type == Equipment_Weapon ? reinterpret_cast(table) : nullptr; } +}; + inline bool VerifyEquipment(flatbuffers::Verifier &verifier, const void *union_obj, Equipment type); MANUALLY_ALIGNED_STRUCT(4) Vec3 FLATBUFFERS_FINAL_CLASS { @@ -347,11 +357,13 @@ inline flatbuffers::Offset EquipmentUnion::Pack(flatbuffers::FlatBufferBui } } -inline EquipmentUnion::~EquipmentUnion() { +inline void EquipmentUnion::Reset() { switch (type) { case Equipment_Weapon: delete reinterpret_cast(table); break; - default:; + default: break; } + table = nullptr; + type = Equipment_NONE; } inline const MyGame::Sample::Monster *GetMonster(const void *buf) { diff --git a/src/idl_gen_cpp.cpp b/src/idl_gen_cpp.cpp index 8681e1749..8e33b59b2 100644 --- a/src/idl_gen_cpp.cpp +++ b/src/idl_gen_cpp.cpp @@ -472,36 +472,6 @@ class CppGenerator : public BaseGenerator { GenTypeBasic(enum_def.underlying_type, false) + ")\n"; code += "\n"; - if (parser_.opts.generate_object_based_api && enum_def.is_union) { - // Generate a union type - code += "struct " + enum_def.name + "Union {\n"; - code += " " + enum_def.name + " type;\n\n"; - code += " flatbuffers::NativeTable *table;\n"; - code += " " + enum_def.name + "Union() : type("; - code += GetEnumValUse(enum_def, *enum_def.vals.Lookup("NONE"), parser_.opts); - code += "), table(nullptr) {}\n"; - code += " " + enum_def.name + "Union(const "; - code += enum_def.name + "Union &);\n"; - code += " " + enum_def.name + "Union &operator=(const "; - code += enum_def.name + "Union &);\n"; - code += " ~" + enum_def.name + "Union();\n\n"; - code += " " + UnionUnPackSignature(enum_def, true) + ";\n"; - code += " " + UnionPackSignature(enum_def, true) + ";\n\n"; - for (auto it = enum_def.vals.vec.begin(); it != enum_def.vals.vec.end(); - ++it) { - auto &ev = **it; - if (ev.value) { - auto native_name = NativeName(WrapInNameSpace(*ev.struct_def)); - code += " " + native_name + " *As"; - code += ev.name + "() { return type == "; - code += GetEnumValUse(enum_def, ev, parser_.opts); - code += " ? reinterpret_cast<" + native_name; - code += " *>(table) : nullptr; }\n"; - } - } - code += "};\n\n"; - } - // Generate a generate string table for enum values. // Problem is, if values are very sparse that could generate really big // tables. Ideally in that case we generate a map lookup instead, but for @@ -552,6 +522,50 @@ class CppGenerator : public BaseGenerator { } } + if (parser_.opts.generate_object_based_api && enum_def.is_union) { + // Generate a union type + code += "struct " + enum_def.name + "Union {\n"; + code += " " + enum_def.name + " type = "; + code += GetEnumValUse(enum_def, *enum_def.vals.Lookup("NONE"), parser_.opts); + code += ";\n\n"; + code += " flatbuffers::NativeTable *table = nullptr;\n"; + code += " " + enum_def.name + "Union() : type("; + code += GetEnumValUse(enum_def, *enum_def.vals.Lookup("NONE"), parser_.opts); + code += "), table(nullptr) {}\n"; + code += " " + enum_def.name + "Union(const "; + code += enum_def.name + "Union &);\n"; + code += " " + enum_def.name + "Union &operator=(const "; + code += enum_def.name + "Union &);\n"; + code += " ~" + enum_def.name + "Union() { Reset(); }\n"; + code += " void Reset();\n\n"; + code += " template \n"; + code += " void Set(T&& value) {\n"; + code += " Reset();\n"; + code += " type = " + enum_def.name; + code += "Traits::enum_value;\n"; + code += " if (type != "; + code += GetEnumValUse(enum_def, *enum_def.vals.Lookup("NONE"), parser_.opts); + code += ") {\n"; + code += " table = new T(std::forward(value));\n"; + code += " }\n"; + code += " }\n\n"; + code += " " + UnionUnPackSignature(enum_def, true) + ";\n"; + code += " " + UnionPackSignature(enum_def, true) + ";\n\n"; + for (auto it = enum_def.vals.vec.begin(); it != enum_def.vals.vec.end(); + ++it) { + auto &ev = **it; + if (ev.value) { + auto native_name = NativeName(WrapInNameSpace(*ev.struct_def)); + code += " " + native_name + " *As"; + code += ev.name + "() { return type == "; + code += GetEnumValUse(enum_def, ev, parser_.opts); + code += " ? reinterpret_cast<" + native_name; + code += " *>(table) : nullptr; }\n"; + } + } + code += "};\n\n"; + } + if (enum_def.is_union) { code += UnionVerifySignature(enum_def) + ";\n\n"; } @@ -614,8 +628,7 @@ class CppGenerator : public BaseGenerator { code += " default: return 0;\n }\n}\n\n"; // Generate a union destructor. - code += "inline " + enum_def.name + "Union::~"; - code += enum_def.name + "Union() {\n"; + code += "inline void " + enum_def.name + "Union::Reset() {\n"; code += " switch (type) {\n"; for (auto it = enum_def.vals.vec.begin(); it != enum_def.vals.vec.end(); ++it) { @@ -627,7 +640,13 @@ class CppGenerator : public BaseGenerator { code += " *>(table); break;\n"; } } - code += " default:;\n }\n}\n\n"; + code += " default: break;\n"; + code += " }\n"; + code += " table = nullptr;\n"; + code += " type = "; + code += GetEnumValUse(enum_def, *enum_def.vals.Lookup("NONE"), parser_.opts); + code += ";\n"; + code += "}\n\n"; } } diff --git a/tests/monster_test_generated.h b/tests/monster_test_generated.h index f1be4ace1..8a2a111e5 100644 --- a/tests/monster_test_generated.h +++ b/tests/monster_test_generated.h @@ -52,23 +52,6 @@ enum Any { Any_MAX = Any_MyGame_Example2_Monster }; -struct AnyUnion { - Any type; - - flatbuffers::NativeTable *table; - AnyUnion() : type(Any_NONE), table(nullptr) {} - AnyUnion(const AnyUnion &); - AnyUnion &operator=(const AnyUnion &); - ~AnyUnion(); - - static flatbuffers::NativeTable *UnPack(const void *union_obj, Any type, const flatbuffers::resolver_function_t *resolver); - flatbuffers::Offset Pack(flatbuffers::FlatBufferBuilder &_fbb, const flatbuffers::rehasher_function_t *rehasher = nullptr) const; - - MonsterT *AsMonster() { return type == Any_Monster ? reinterpret_cast(table) : nullptr; } - TestSimpleTableWithEnumT *AsTestSimpleTableWithEnum() { return type == Any_TestSimpleTableWithEnum ? reinterpret_cast(table) : nullptr; } - MyGame::Example2::MonsterT *AsMyGame_Example2_Monster() { return type == Any_MyGame_Example2_Monster ? reinterpret_cast(table) : nullptr; } -}; - inline const char **EnumNamesAny() { static const char *names[] = { "NONE", "Monster", "TestSimpleTableWithEnum", "MyGame_Example2_Monster", nullptr }; return names; @@ -92,6 +75,33 @@ template<> struct AnyTraits { static const Any enum_value = Any_MyGame_Example2_Monster; }; +struct AnyUnion { + Any type = Any_NONE; + + flatbuffers::NativeTable *table = nullptr; + AnyUnion() : type(Any_NONE), table(nullptr) {} + AnyUnion(const AnyUnion &); + AnyUnion &operator=(const AnyUnion &); + ~AnyUnion() { Reset(); } + void Reset(); + + template + void Set(T&& value) { + Reset(); + type = AnyTraits::enum_value; + if (type != Any_NONE) { + table = new T(std::move(value)); + } + } + + static flatbuffers::NativeTable *UnPack(const void *union_obj, Any type, const flatbuffers::resolver_function_t *resolver); + flatbuffers::Offset Pack(flatbuffers::FlatBufferBuilder &_fbb, const flatbuffers::rehasher_function_t *rehasher = nullptr) const; + + MonsterT *AsMonster() { return type == Any_Monster ? reinterpret_cast(table) : nullptr; } + TestSimpleTableWithEnumT *AsTestSimpleTableWithEnum() { return type == Any_TestSimpleTableWithEnum ? reinterpret_cast(table) : nullptr; } + MyGame::Example2::MonsterT *AsMyGame_Example2_Monster() { return type == Any_MyGame_Example2_Monster ? reinterpret_cast(table) : nullptr; } +}; + inline bool VerifyAny(flatbuffers::Verifier &verifier, const void *union_obj, Any type); MANUALLY_ALIGNED_STRUCT(2) Test FLATBUFFERS_FINAL_CLASS { @@ -767,13 +777,15 @@ inline flatbuffers::Offset AnyUnion::Pack(flatbuffers::FlatBufferBuilder & } } -inline AnyUnion::~AnyUnion() { +inline void AnyUnion::Reset() { switch (type) { case Any_Monster: delete reinterpret_cast(table); break; case Any_TestSimpleTableWithEnum: delete reinterpret_cast(table); break; case Any_MyGame_Example2_Monster: delete reinterpret_cast(table); break; - default:; + default: break; } + table = nullptr; + type = Any_NONE; } inline const MyGame::Example::Monster *GetMonster(const void *buf) {