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.

Sign up for GitHub

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

Fix issues with uint64 enums #5265

Merged
merged 3 commits into from
May 2, 2019
Merged

Fix issues with uint64 enums #5265

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89c6eee
Fix issues with uint64 enums
vglavnyy Apr 12, 2019
f1f6081
Refine objectives of EnumDef's FindByValue and ReverseLookup methods
vglavnyy May 1, 2019
5823102
Make the IsUInt64 method private
vglavnyy May 1, 2019
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
5 changes: 5 additions & 0 deletions include/flatbuffers/code_generators.h
View file
Open in desktop
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
78 changes: 69 additions & 9 deletions include/flatbuffers/idl.h
View file
Open in desktop
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,105 @@ 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); }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I appreciate that this refactor is cleaner, we also have to think about backwards compatibility, since this class is a public API. In that way, I'd prefer for value to keep existing for the signed case, and there to be a method to get the unsigned one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is no big deal for people that access value to replace it with your new method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnumVal and EnumDef are internal public. They are not used outside in the external (generated) code.

int64_t GetAsInt64() const { return value; }
bool IsZero() const { return 0 == value; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two methods are nicely descriptive, but a bit superfluous, as using an int as a bool is fine in C++

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsZero and IsNotZero were added for visibility during refactoring.
The 'value` became private and I left these methods.

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() +
for (auto it = Vals().begin() +
static_cast<int>(is_union && skip_union_default);
it != Vals().end(); ++it) {
if ((*it)->value == enum_idx) { return *it; }
if ((*it)->GetAsInt64() == 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()); }

// In fact, this is uint64/int64 safe ReverseLookup without
// skip_union_default.
EnumVal *FindByValue(const std::string &constant) const;

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

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:
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 Expand Up @@ -705,7 +765,7 @@ class Parser : public ParserState {

FLATBUFFERS_CHECKED_ERROR Error(const std::string &msg);

private:
vglavnyy marked this conversation as resolved.
Show resolved Hide resolved
private:
void Message(const std::string &msg);
void Warning(const std::string &msg);
FLATBUFFERS_CHECKED_ERROR ParseHexNum(int nibbles, uint64_t *val);
Expand Down
106 changes: 68 additions & 38 deletions src/idl_gen_cpp.cpp
View file
Open in desktop
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 greate 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 explicit declared enum type.
// The value 4611686018427387904ULL 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