Skip to content

Commit

Permalink
Fix issues with uint64 enums (#5265)
Browse files Browse the repository at this point in the history 
* Fix issues with uint64 enums

- hide the implementation of enums from code generators
- fix uint64 the issue in the cpp-generator
- fix #5108
- new tests
- enums with bit_flags attribute should be unsigned

* Refine objectives of EnumDef's FindByValue and ReverseLookup methods

- move EnumDef::ReverseLookup implementation to idl_parser.cpp
- fix typos

* Make the IsUInt64 method private
  • Loading branch information
@vglavnyy @aardappel
vglavnyy authored and aardappel committed May 2, 2019
1 parent 6cc30b3 commit b8ef8c1
Show file tree
Hide file tree
Showing 16 changed files with 526 additions and 276 deletions.
5 changes: 5 additions & 0 deletions include/flatbuffers/code_generators.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ class CodeWriter {
value_map_[key] = value;
}

std::string GetValue(const std::string &key) const {
const auto it = value_map_.find(key);
return it == value_map_.end() ? "" : it->second;
}

// Appends the given text to the generated code as well as a newline
// character. Any text within {{ and }} delimeters is replaced by values
// previously stored in the CodeWriter by calling SetValue above. The newline
Expand Down
81 changes: 66 additions & 15 deletions include/flatbuffers/idl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,13 @@ inline bool IsLong (BaseType t) { return t == BASE_TYPE_LONG ||
inline bool IsBool (BaseType t) { return t == BASE_TYPE_BOOL; }
inline bool IsOneByte(BaseType t) { return t >= BASE_TYPE_UTYPE &&
t <= BASE_TYPE_UCHAR; }

inline bool IsUnsigned(BaseType t) {
return (t == BASE_TYPE_UTYPE) || (t == BASE_TYPE_UCHAR) ||
(t == BASE_TYPE_USHORT) || (t == BASE_TYPE_UINT) ||
(t == BASE_TYPE_ULONG);
}

// clang-format on

extern const char *const kTypeNames[];
Expand Down Expand Up @@ -327,52 +334,96 @@ inline size_t InlineAlignment(const Type &type) {
return IsStruct(type) ? type.struct_def->minalign : SizeOf(type.base_type);
}

struct EnumVal {
EnumVal(const std::string &_name, int64_t _val) : name(_name), value(_val) {}
EnumVal() : value(0) {}
struct EnumDef;
struct EnumValBuilder;

struct EnumVal {
Offset<reflection::EnumVal> Serialize(FlatBufferBuilder *builder, const Parser &parser) const;

bool Deserialize(const Parser &parser, const reflection::EnumVal *val);

uint64_t GetAsUInt64() const { return static_cast<uint64_t>(value); }
int64_t GetAsInt64() const { return value; }
bool IsZero() const { return 0 == value; }
bool IsNonZero() const { return !IsZero(); }

std::string name;
std::vector<std::string> doc_comment;
int64_t value;
Type union_type;

private:
friend EnumDef;
friend EnumValBuilder;
friend bool operator==(const EnumVal &lhs, const EnumVal &rhs);

EnumVal(const std::string &_name, int64_t _val) : name(_name), value(_val) {}
EnumVal() : value(0) {}

int64_t value;
};

struct EnumDef : public Definition {
EnumDef() : is_union(false), uses_multiple_type_instances(false) {}

EnumVal *ReverseLookup(int64_t enum_idx, bool skip_union_default = true) {
for (auto it = Vals().begin() +
static_cast<int>(is_union && skip_union_default);
it != Vals().end(); ++it) {
if ((*it)->value == enum_idx) { return *it; }
}
return nullptr;
}

Offset<reflection::Enum> Serialize(FlatBufferBuilder *builder, const Parser &parser) const;
Offset<reflection::Enum> Serialize(FlatBufferBuilder *builder,
const Parser &parser) const;

bool Deserialize(Parser &parser, const reflection::Enum *values);

template<typename T> void ChangeEnumValue(EnumVal *ev, T new_val);
void SortByValue();
void RemoveDuplicates();

std::string AllFlags() const;
const EnumVal *MinValue() const;
const EnumVal *MaxValue() const;
// Returns the number of integer steps from v1 to v2.
uint64_t Distance(const EnumVal *v1, const EnumVal *v2) const;
// Returns the number of integer steps from Min to Max.
uint64_t Distance() const { return Distance(MinValue(), MaxValue()); }

EnumVal *ReverseLookup(int64_t enum_idx,
bool skip_union_default = false) const;
EnumVal *FindByValue(const std::string &constant) const;

std::string ToString(const EnumVal &ev) const {
return IsUInt64() ? NumToString(ev.GetAsUInt64())
: NumToString(ev.GetAsInt64());
}

size_t size() const { return vals.vec.size(); }

const std::vector<EnumVal *> &Vals() const {
FLATBUFFERS_ASSERT(false == vals.vec.empty());
return vals.vec;
}

SymbolTable<EnumVal> vals;
const EnumVal *Lookup(const std::string &enum_name) const {
return vals.Lookup(enum_name);
}

bool is_union;
// Type is a union which uses type aliases where at least one type is
// available under two different names.
bool uses_multiple_type_instances;
Type underlying_type;

private:
bool IsUInt64() const {
return (BASE_TYPE_ULONG == underlying_type.base_type);
}

friend EnumValBuilder;
SymbolTable<EnumVal> vals;
};

inline bool operator==(const EnumVal &lhs, const EnumVal &rhs) {
return lhs.value == rhs.value;
}
inline bool operator!=(const EnumVal &lhs, const EnumVal &rhs) {
return !(lhs == rhs);
}

inline bool EqualByName(const Type &a, const Type &b) {
return a.base_type == b.base_type && a.element == b.element &&
(a.struct_def == b.struct_def ||
Expand Down
106 changes: 68 additions & 38 deletions src/idl_gen_cpp.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,23 @@ namespace flatbuffers {
// Pedantic warning free version of toupper().
inline char ToUpper(char c) { return static_cast<char>(::toupper(c)); }

// Make numerical literal with type-suffix.
// This function is only needed for C++! Other languages do not need it.
static inline std::string NumToStringCpp(std::string val, BaseType type) {
// Avoid issues with -2147483648, -9223372036854775808.
switch (type) {
case BASE_TYPE_INT:
return (val != "-2147483648") ? val : ("(-2147483647 - 1)");
case BASE_TYPE_ULONG: return (val == "0") ? val : (val + "ULL");
case BASE_TYPE_LONG:
if (val == "-9223372036854775808")
return "(-9223372036854775807LL - 1LL)";
else
return (val == "0") ? val : (val + "LL");
default: return val;
}
}

static std::string GeneratedFileName(const std::string &path,
const std::string &file_name) {
return path + file_name + "_generated.h";
Expand Down Expand Up @@ -789,7 +806,7 @@ class CppGenerator : public BaseGenerator {
code_.SetValue("NUM_FIELDS", NumToString(num_fields));
std::vector<std::string> names;
std::vector<Type> types;
bool consecutive_enum_from_zero = true;

if (struct_def) {
for (auto it = struct_def->fields.vec.begin();
it != struct_def->fields.vec.end(); ++it) {
Expand All @@ -804,9 +821,6 @@ class CppGenerator : public BaseGenerator {
names.push_back(Name(ev));
types.push_back(enum_def->is_union ? ev.union_type
: Type(enum_def->underlying_type));
if (static_cast<int64_t>(it - enum_def->Vals().begin()) != ev.value) {
consecutive_enum_from_zero = false;
}
}
}
std::string ts;
Expand Down Expand Up @@ -851,12 +865,16 @@ class CppGenerator : public BaseGenerator {
ns += "\"" + *it + "\"";
}
std::string vs;
const auto consecutive_enum_from_zero =
enum_def && enum_def->MinValue()->IsZero() &&
((enum_def->size() - 1) == enum_def->Distance());
if (enum_def && !consecutive_enum_from_zero) {
for (auto it = enum_def->Vals().begin(); it != enum_def->Vals().end();
++it) {
const auto &ev = **it;
if (!vs.empty()) vs += ", ";
vs += NumToString(ev.value);
vs += NumToStringCpp(enum_def->ToString(ev),
enum_def->underlying_type.base_type);
}
} else if (struct_def && struct_def->fixed) {
for (auto it = struct_def->fields.vec.begin();
Expand All @@ -883,6 +901,7 @@ class CppGenerator : public BaseGenerator {
code_ += " };";
}
if (!vs.empty()) {
// Problem with uint64_t values greater than 9223372036854775807ULL.
code_ += " static const int64_t values[] = { {{VALUES}} };";
}
auto has_names =
Expand All @@ -907,31 +926,39 @@ class CppGenerator : public BaseGenerator {
// Generate an enum declaration,
// an enum string lookup table,
// and an enum array of values

void GenEnum(const EnumDef &enum_def) {
code_.SetValue("ENUM_NAME", Name(enum_def));
code_.SetValue("BASE_TYPE", GenTypeBasic(enum_def.underlying_type, false));
code_.SetValue("SEP", "");

GenComment(enum_def.doc_comment);
code_ += GenEnumDecl(enum_def) + "\\";
if (parser_.opts.scoped_enums) code_ += " : {{BASE_TYPE}}\\";
// MSVC doesn't support int64/uint64 enum without explicitly declared enum
// type. The value 4611686018427387904ULL is truncated to zero with warning:
// "warning C4309: 'initializing': truncation of constant value".
auto add_type = parser_.opts.scoped_enums;
add_type |= (enum_def.underlying_type.base_type == BASE_TYPE_LONG);
add_type |= (enum_def.underlying_type.base_type == BASE_TYPE_ULONG);
if (add_type) code_ += " : {{BASE_TYPE}}\\";
code_ += " {";

int64_t anyv = 0;
const EnumVal *minv = nullptr, *maxv = nullptr;
for (auto it = enum_def.Vals().begin(); it != enum_def.Vals().end(); ++it) {
const auto &ev = **it;

GenComment(ev.doc_comment, " ");
if (!ev.doc_comment.empty()) {
auto prefix = code_.GetValue("SEP") + " ";
GenComment(ev.doc_comment, prefix.c_str());
code_.SetValue("SEP", "");
}
code_.SetValue("KEY", GenEnumValDecl(enum_def, Name(ev)));
code_.SetValue("VALUE", NumToString(ev.value));
code_.SetValue("VALUE",
NumToStringCpp(enum_def.ToString(ev),
enum_def.underlying_type.base_type));
code_ += "{{SEP}} {{KEY}} = {{VALUE}}\\";
code_.SetValue("SEP", ",\n");

minv = !minv || minv->value > ev.value ? &ev : minv;
maxv = !maxv || maxv->value < ev.value ? &ev : maxv;
anyv |= ev.value;
}
const EnumVal *minv = enum_def.MinValue();
const EnumVal *maxv = enum_def.MaxValue();

if (parser_.opts.scoped_enums || parser_.opts.prefixed_enums) {
FLATBUFFERS_ASSERT(minv && maxv);
Expand All @@ -943,7 +970,9 @@ class CppGenerator : public BaseGenerator {
code_ += "{{SEP}} {{KEY}} = {{VALUE}}\\";

code_.SetValue("KEY", GenEnumValDecl(enum_def, "ANY"));
code_.SetValue("VALUE", NumToString(anyv));
code_.SetValue("VALUE",
NumToStringCpp(enum_def.AllFlags(),
enum_def.underlying_type.base_type));
code_ += "{{SEP}} {{KEY}} = {{VALUE}}\\";
} else { // MIN & MAX are useless for bit_flags
code_.SetValue("KEY", GenEnumValDecl(enum_def, "MIN"));
Expand Down Expand Up @@ -984,22 +1013,23 @@ class CppGenerator : public BaseGenerator {
// 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
// the moment we simply don't output a table at all.
auto range =
enum_def.vals.vec.back()->value - enum_def.vals.vec.front()->value + 1;
auto range = enum_def.Distance();
// Average distance between values above which we consider a table
// "too sparse". Change at will.
static const int kMaxSparseness = 5;
if (range / static_cast<int64_t>(enum_def.vals.vec.size()) <
kMaxSparseness) {
static const uint64_t kMaxSparseness = 5;
if (range / static_cast<uint64_t>(enum_def.size()) < kMaxSparseness) {
code_ += "inline const char * const *EnumNames{{ENUM_NAME}}() {";
code_ += " static const char * const names[] = {";

auto val = enum_def.Vals().front()->value;
auto val = enum_def.Vals().front();
for (auto it = enum_def.Vals().begin(); it != enum_def.Vals().end();
++it) {
const auto &ev = **it;
while (val++ != ev.value) { code_ += " \"\","; }
code_ += " \"" + Name(ev) + "\",";
auto ev = *it;
for (auto k = enum_def.Distance(val, ev); k > 1; --k) {
code_ += " \"\",";
}
val = ev;
code_ += " \"" + Name(*ev) + "\",";
}
code_ += " nullptr";
code_ += " };";
Expand All @@ -1010,14 +1040,13 @@ class CppGenerator : public BaseGenerator {

code_ += "inline const char *EnumName{{ENUM_NAME}}({{ENUM_NAME}} e) {";

code_ += " if (e < " +
GetEnumValUse(enum_def, *enum_def.vals.vec.front()) +
" || e > " + GetEnumValUse(enum_def, *enum_def.vals.vec.back()) +
code_ += " if (e < " + GetEnumValUse(enum_def, *enum_def.MinValue()) +
" || e > " + GetEnumValUse(enum_def, *enum_def.MaxValue()) +
") return \"\";";

code_ += " const size_t index = static_cast<size_t>(e)\\";
if (enum_def.vals.vec.front()->value) {
auto vals = GetEnumValUse(enum_def, *enum_def.vals.vec.front());
if (enum_def.MinValue()->IsNonZero()) {
auto vals = GetEnumValUse(enum_def, *enum_def.MinValue());
code_ += " - static_cast<size_t>(" + vals + ")\\";
}
code_ += ";";
Expand Down Expand Up @@ -1067,8 +1096,8 @@ class CppGenerator : public BaseGenerator {
if (parser_.opts.generate_object_based_api && enum_def.is_union) {
// Generate a union type
code_.SetValue("NAME", Name(enum_def));
code_.SetValue("NONE",
GetEnumValUse(enum_def, *enum_def.vals.Lookup("NONE")));
FLATBUFFERS_ASSERT(enum_def.Lookup("NONE"));
code_.SetValue("NONE", GetEnumValUse(enum_def, *enum_def.Lookup("NONE")));

code_ += "struct {{NAME}}Union {";
code_ += " {{NAME}} type;";
Expand Down Expand Up @@ -1363,8 +1392,8 @@ class CppGenerator : public BaseGenerator {
code_ += "";

// Union Reset() function.
code_.SetValue("NONE",
GetEnumValUse(enum_def, *enum_def.vals.Lookup("NONE")));
FLATBUFFERS_ASSERT(enum_def.Lookup("NONE"));
code_.SetValue("NONE", GetEnumValUse(enum_def, *enum_def.Lookup("NONE")));

code_ += "inline void {{ENUM_NAME}}Union::Reset() {";
code_ += " switch (type) {";
Expand Down Expand Up @@ -1430,18 +1459,19 @@ class CppGenerator : public BaseGenerator {
if (IsFloat(field.value.type.base_type))
return float_const_gen_.GenFloatConstant(field);
else
return field.value.constant;
return NumToStringCpp(field.value.constant, field.value.type.base_type);
}

std::string GetDefaultScalarValue(const FieldDef &field, bool is_ctor) {
if (field.value.type.enum_def && IsScalar(field.value.type.base_type)) {
auto ev = field.value.type.enum_def->ReverseLookup(
StringToInt(field.value.constant.c_str()), false);
auto ev = field.value.type.enum_def->FindByValue(field.value.constant);
if (ev) {
return WrapInNameSpace(field.value.type.enum_def->defined_namespace,
GetEnumValUse(*field.value.type.enum_def, *ev));
} else {
return GenUnderlyingCast(field, true, field.value.constant);
return GenUnderlyingCast(
field, true,
NumToStringCpp(field.value.constant, field.value.type.base_type));
}
} else if (field.value.type.base_type == BASE_TYPE_BOOL) {
return field.value.constant == "0" ? "false" : "true";
Expand Down
Loading

0 comments on commit b8ef8c1

Please sign in to comment.