Skip to content

Commit

Permalink
UnPackTo disable merge by default (#7527)
Browse files Browse the repository at this point in the history 
* UnPackTo disable merge by default

* avoid double free in test

* remove merge parameter

* remove shrink to fit
  • Loading branch information
@dbaileychess
dbaileychess committed Sep 14, 2022
1 parent 4fca4dc commit bc44fad
Show file tree
Hide file tree
Showing 16 changed files with 202 additions and 206 deletions.
4 changes: 2 additions & 2 deletions samples/monster_generated.h
View file
Expand Up @@ -607,10 +607,10 @@ inline void Monster::UnPackTo(MonsterT *_o, const flatbuffers::resolver_function
{ auto _e = name(); if (_e) _o->name = _e->str(); }
{ auto _e = inventory(); if (_e) { _o->inventory.resize(_e->size()); std::copy(_e->begin(), _e->end(), _o->inventory.begin()); } }
{ auto _e = color(); _o->color = _e; }
{ auto _e = weapons(); if (_e) { _o->weapons.resize(_e->size()); for (flatbuffers::uoffset_t _i = 0; _i < _e->size(); _i++) { _o->weapons[_i] = flatbuffers::unique_ptr<MyGame::Sample::WeaponT>(_e->Get(_i)->UnPack(_resolver)); } } }
{ auto _e = weapons(); if (_e) { _o->weapons.resize(_e->size()); for (flatbuffers::uoffset_t _i = 0; _i < _e->size(); _i++) { if(_o->weapons[_i]) { _e->Get(_i)->UnPackTo(_o->weapons[_i].get(), _resolver); } else { _o->weapons[_i] = flatbuffers::unique_ptr<MyGame::Sample::WeaponT>(_e->Get(_i)->UnPack(_resolver)); }; } } else { _o->weapons.resize(0); } }
{ auto _e = equipped_type(); _o->equipped.type = _e; }
{ auto _e = equipped(); if (_e) _o->equipped.value = MyGame::Sample::EquipmentUnion::UnPack(_e, equipped_type(), _resolver); }
{ auto _e = path(); if (_e) { _o->path.resize(_e->size()); for (flatbuffers::uoffset_t _i = 0; _i < _e->size(); _i++) { _o->path[_i] = *_e->Get(_i); } } }
{ auto _e = path(); if (_e) { _o->path.resize(_e->size()); for (flatbuffers::uoffset_t _i = 0; _i < _e->size(); _i++) { _o->path[_i] = *_e->Get(_i); } } else { _o->path.resize(0); } }
}

inline flatbuffers::Offset<Monster> Monster::Pack(flatbuffers::FlatBufferBuilder &_fbb, const MonsterT* _o, const flatbuffers::rehasher_function_t *_rehasher) {
Expand Down
61 changes: 29 additions & 32 deletions src/idl_gen_cpp.cpp
View file
Expand Up @@ -26,10 +26,6 @@
#include "flatbuffers/idl.h"
#include "flatbuffers/util.h"

#ifndef FLATBUFFERS_CPP_OBJECT_UNPACKTO
#define FLATBUFFERS_CPP_OBJECT_UNPACKTO 0
#endif

namespace flatbuffers {

// Make numerical literal with type-suffix.
Expand Down Expand Up @@ -74,13 +70,17 @@ static std::string GenIncludeGuard(const std::string &file_name,
return guard;
}

static bool IsVectorOfPointers(const FieldDef& field) {
const auto& type = field.value.type;
const auto& vector_type = type.VectorType();
static bool IsVectorOfPointers(const FieldDef &field) {
const auto &type = field.value.type;
const auto &vector_type = type.VectorType();
return type.base_type == BASE_TYPE_VECTOR &&
vector_type.base_type == BASE_TYPE_STRUCT &&
!vector_type.struct_def->fixed &&
!field.native_inline;
!vector_type.struct_def->fixed && !field.native_inline;
}

static bool IsPointer(const FieldDef &field) {
return field.value.type.base_type == BASE_TYPE_STRUCT &&
!IsStruct(field.value.type);
}

namespace cpp {
Expand Down Expand Up @@ -1882,7 +1882,8 @@ class CppGenerator : public BaseGenerator {
if (vec_type.base_type == BASE_TYPE_UTYPE) continue;
const auto cpp_type = field.attributes.Lookup("cpp_type");
const auto cpp_ptr_type = field.attributes.Lookup("cpp_ptr_type");
const bool is_ptr = IsVectorOfPointers(field) || (cpp_type && cpp_ptr_type->constant != "naked");
const bool is_ptr = IsVectorOfPointers(field) ||
(cpp_type && cpp_ptr_type->constant != "naked");
if (is_ptr) { return true; }
}
}
Expand Down Expand Up @@ -2006,7 +2007,8 @@ class CppGenerator : public BaseGenerator {
? cpp_type->constant
: GenTypeNative(vec_type, /*invector*/ true,
field, /*forcopy*/ true);
const bool is_ptr = IsVectorOfPointers(field) || (cpp_type && cpp_ptr_type->constant != "naked");
const bool is_ptr = IsVectorOfPointers(field) ||
(cpp_type && cpp_ptr_type->constant != "naked");
CodeWriter cw(" ");
cw.SetValue("FIELD", Name(field));
cw.SetValue("TYPE", type_name);
Expand Down Expand Up @@ -3013,7 +3015,8 @@ class CppGenerator : public BaseGenerator {
if (field.value.type.element == BASE_TYPE_UTYPE) {
name = StripUnionType(Name(field));
}
code += "{ _o->" + name + ".resize(_e->size()); ";
const std::string vector_field = "_o->" + name;
code += "{ " + vector_field + ".resize(_e->size()); ";

This comment has been minimized.

Sign in to view

Copy link
@Naios

Naios Sep 28, 2022

Contributor

@dbaileychess I noticed that this is not correct for fixed arrays.

The FB compiler generates something like { auto const _e = speed(); { _o->speed.resize(_e->size()); for (flatbuffers::uoffset_t _i = 0; _i < _e->size(); _i++) { _o->speed[_i] = _e->Get(_i); } } else { _o->speed.resize(0); } } for speed: [float: 4].
if (!field.value.type.enum_def && !IsBool(field.value.type.element) &&
IsOneByte(field.value.type.element)) {
// For vectors of bytes, std::copy is used to improve performance.
Expand Down Expand Up @@ -3053,7 +3056,7 @@ class CppGenerator : public BaseGenerator {
// (*resolver)(&_o->field, (hash_value_t)(_e));
// else
// _o->field = nullptr;
code += "//vector resolver, " + PtrType(&field) + "\n";
code += "/*vector resolver, " + PtrType(&field) + "*/ ";
code += "if (_resolver) ";
code += "(*_resolver)";
code += "(reinterpret_cast<void **>(&_o->" + name + "[_i]" +
Expand All @@ -3070,25 +3073,19 @@ class CppGenerator : public BaseGenerator {
code += "/* else do nothing */";
}
} else {
// clang-format off
#if FLATBUFFERS_CPP_OBJECT_UNPACKTO
const bool is_pointer = IsVectorOfPointers(field);
if (is_pointer) {
code += "if(_o->" + name + "[_i]" + ") { ";
code += indexing + "->UnPackTo(_o->" + name +
"[_i].get(), _resolver);";
code += " } else { ";
}
#endif
code += "_o->" + name + "[_i]" + access + " = ";
code += GenUnpackVal(field.value.type.VectorType(), indexing, true,
field);
#if FLATBUFFERS_CPP_OBJECT_UNPACKTO
if (is_pointer) { code += "; }"; }
#endif
// clang-format on
}
code += "; } }";
code += "; } } else { " + vector_field + ".resize(0); }";
}
break;
}
Expand Down Expand Up @@ -3116,7 +3113,7 @@ class CppGenerator : public BaseGenerator {
// (*resolver)(&_o->field, (hash_value_t)(_e));
// else
// _o->field = nullptr;
code += "//scalar resolver, " + PtrType(&field) + " \n";
code += "/*scalar resolver, " + PtrType(&field) + "*/ ";
code += "if (_resolver) ";
code += "(*_resolver)";
code += "(reinterpret_cast<void **>(&_o->" + Name(field) + "), ";
Expand All @@ -3133,21 +3130,21 @@ class CppGenerator : public BaseGenerator {
} else {
// Generate code for assigning the value, of the form:
// _o->field = value;
// clang-format off
#if FLATBUFFERS_CPP_OBJECT_UNPACKTO
const bool is_pointer = IsVectorOfPointers(field);
const bool is_pointer = IsPointer(field);

const std::string out_field = "_o->" + Name(field);

if (is_pointer) {
code += "{ if(_o->" + Name(field) + ") { ";
code += "_e->UnPackTo(_o->" + Name(field) + ".get(), _resolver);";
code += "{ if(" + out_field + ") { ";
code += "_e->UnPackTo(" + out_field + ".get(), _resolver);";
code += " } else { ";
}
#endif
code += "_o->" + Name(field) + " = ";
code += out_field + " = ";
code += GenUnpackVal(field.value.type, "_e", false, field) + ";";
#if FLATBUFFERS_CPP_OBJECT_UNPACKTO
if (is_pointer) { code += " } }"; }
#endif
// clang-format on
if (is_pointer) {
code += " } } else if (" + out_field + ") { " + out_field +
".reset(); }";
}
}
break;
}
Expand Down

0 comments on commit bc44fad

Please sign in to comment.