From ae0b22cc29a329f5e094c37d8fff166d01b6fab5 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Thu, 4 Sep 2014 23:04:21 +0200 Subject: THRIFT-2513 clean up enum value assignment Patch: Dave Watson This closes #88 Summary: Clean up how enum values are handled if an integer value is not explicitly specified in the thrift file. For example, the following used to be a compile error, but works now: enum MyEnum { SOMEVALUE } struct MyStruct { 1: MyEnum e = SOMEVALUE } This change also cleans up some of the error handling with out-of-range values. Previously thrift simply issued a warning for enum values that didn't fit in an i32, but serialized them as i32 anyway. Now out-of-range enum values result in a compile failure. Test Plan: Included a new unit test to verify the assignment of enum values. I also verified that g++ makes the same enum value assignments when compiling these enums as C++ code. --- compiler/cpp/src/generate/t_c_glib_generator.cc | 6 +- compiler/cpp/src/generate/t_cpp_generator.cc | 2 +- compiler/cpp/src/generate/t_d_generator.cc | 4 +- compiler/cpp/src/generate/t_go_generator.cc | 6 +- compiler/cpp/src/parse/t_enum.h | 23 ++------ compiler/cpp/src/parse/t_enum_value.h | 20 +------ compiler/cpp/src/thrifty.yy | 75 +++++++++++++++++-------- 7 files changed, 62 insertions(+), 74 deletions(-) (limited to 'compiler/cpp') diff --git a/compiler/cpp/src/generate/t_c_glib_generator.cc b/compiler/cpp/src/generate/t_c_glib_generator.cc index 96ce99e20..c726b0c5a 100644 --- a/compiler/cpp/src/generate/t_c_glib_generator.cc +++ b/compiler/cpp/src/generate/t_c_glib_generator.cc @@ -314,10 +314,8 @@ void t_c_glib_generator::generate_enum(t_enum *tenum) { f_types_ << indent() << this->nspace_uc << name_uc << "_" << (*c_iter)->get_name(); - if ((*c_iter)->has_value()) { - f_types_ << - " = " << (*c_iter)->get_value(); - } + f_types_ << + " = " << (*c_iter)->get_value(); } indent_down(); diff --git a/compiler/cpp/src/generate/t_cpp_generator.cc b/compiler/cpp/src/generate/t_cpp_generator.cc index 85eeeef5b..f019ba86a 100755 --- a/compiler/cpp/src/generate/t_cpp_generator.cc +++ b/compiler/cpp/src/generate/t_cpp_generator.cc @@ -524,7 +524,7 @@ void t_cpp_generator::generate_enum_constant_list(std::ofstream& f, } indent(f) << prefix << (*c_iter)->get_name() << suffix; - if (include_values && (*c_iter)->has_value()) { + if (include_values) { f << " = " << (*c_iter)->get_value(); } } diff --git a/compiler/cpp/src/generate/t_d_generator.cc b/compiler/cpp/src/generate/t_d_generator.cc index 706506af2..ecf908bdd 100644 --- a/compiler/cpp/src/generate/t_d_generator.cc +++ b/compiler/cpp/src/generate/t_d_generator.cc @@ -188,9 +188,7 @@ class t_d_generator : public t_oop_generator { f_types_ << "," << endl; } indent(f_types_) << (*c_iter)->get_name(); - if ((*c_iter)->has_value()) { - f_types_ << " = " << (*c_iter)->get_value(); - } + f_types_ << " = " << (*c_iter)->get_value(); } f_types_ << endl; diff --git a/compiler/cpp/src/generate/t_go_generator.cc b/compiler/cpp/src/generate/t_go_generator.cc index bcd7e5e83..9a3e9099e 100644 --- a/compiler/cpp/src/generate/t_go_generator.cc +++ b/compiler/cpp/src/generate/t_go_generator.cc @@ -829,11 +829,7 @@ void t_go_generator::generate_enum(t_enum* tenum) int value = -1; for (c_iter = constants.begin(); c_iter != constants.end(); ++c_iter) { - if ((*c_iter)->has_value()) { - value = (*c_iter)->get_value(); - } else { - ++value; - } + value = (*c_iter)->get_value(); string iter_std_name(escape_string((*c_iter)->get_name())); string iter_name((*c_iter)->get_name()); diff --git a/compiler/cpp/src/parse/t_enum.h b/compiler/cpp/src/parse/t_enum.h index ec847d16b..94cb26ea5 100644 --- a/compiler/cpp/src/parse/t_enum.h +++ b/compiler/cpp/src/parse/t_enum.h @@ -76,10 +76,9 @@ class t_enum : public t_type { else { int min_value_value; min_value = enum_values.front(); - min_value_value = (min_value->has_value() ? min_value->get_value() : 1); + min_value_value = min_value->get_value(); for (c_iter = enum_values.begin(); c_iter != enum_values.end(); ++c_iter) { - if ((*c_iter)->has_value() && - ((*c_iter)->get_value() < min_value_value)) { + if ((*c_iter)->get_value() < min_value_value) { min_value = (*c_iter); min_value_value = min_value->get_value(); } @@ -98,11 +97,9 @@ class t_enum : public t_type { else { int max_value_value; max_value = enum_values.back(); - max_value_value = - (max_value->has_value() ? max_value->get_value() : enum_values.size()); + max_value_value = max_value->get_value(); for (c_iter = enum_values.begin(); c_iter != enum_values.end(); ++c_iter) { - if ((*c_iter)->has_value() && - ((*c_iter)->get_value() > max_value_value)) { + if ((*c_iter)->get_value() > max_value_value) { max_value = (*c_iter); max_value_value = max_value->get_value(); } @@ -119,18 +116,6 @@ class t_enum : public t_type { return "enum"; } - void resolve_values() { - const std::vector& enum_values = get_constants(); - std::vector::const_iterator c_iter; - int lastValue = -1; - for (c_iter = enum_values.begin(); c_iter != enum_values.end(); ++c_iter) { - if (! (*c_iter)->has_value()) { - (*c_iter)->set_value(++lastValue); - } else { - lastValue = (*c_iter)->get_value(); - } - } - } private: std::vector constants_; diff --git a/compiler/cpp/src/parse/t_enum_value.h b/compiler/cpp/src/parse/t_enum_value.h index 3a4a90a83..26798d733 100644 --- a/compiler/cpp/src/parse/t_enum_value.h +++ b/compiler/cpp/src/parse/t_enum_value.h @@ -31,40 +31,24 @@ */ class t_enum_value : public t_doc { public: - t_enum_value(std::string name) : - name_(name), - has_value_(false), - value_(0) {} - t_enum_value(std::string name, int value) : name_(name), - has_value_(true), value_(value) {} ~t_enum_value() {} - const std::string& get_name() { + const std::string& get_name() const { return name_; } - bool has_value() { - return has_value_; - } - - int get_value() { + int get_value() const { return value_; } - void set_value(int val) { - has_value_ = true; - value_ = val; - } - std::map annotations_; private: std::string name_; - bool has_value_; int value_; }; diff --git a/compiler/cpp/src/thrifty.yy b/compiler/cpp/src/thrifty.yy index da5c562f9..3755d134c 100644 --- a/compiler/cpp/src/thrifty.yy +++ b/compiler/cpp/src/thrifty.yy @@ -53,6 +53,13 @@ * assigned starting from -1 and working their way down. */ int y_field_val = -1; +/** + * This global variable is used for automatic numbering of enum values. + * y_enum_val is the last value assigned; the next auto-assigned value will be + * y_enum_val+1, and then it continues working upwards. Explicitly specified + * enum values reset y_enum_val to that value. + */ +int32_t y_enum_val = -1; int g_arglist = 0; const int struct_is_struct = 0; const int struct_is_union = 1; @@ -200,6 +207,7 @@ const int struct_is_union = 1; %type Enum %type EnumDefList %type EnumDef +%type EnumValue %type Senum %type SenumDefList @@ -569,7 +577,7 @@ Enum: $$->annotations_ = $6->annotations_; delete $6; } - $$->resolve_values(); + // make constants for all the enum values if (g_parse_mode == PROGRAM) { const std::vector& enum_values = $$->get_constants(); @@ -597,36 +605,28 @@ EnumDefList: { pdebug("EnumDefList -> "); $$ = new t_enum(g_program); + y_enum_val = -1; } EnumDef: - CaptureDocText tok_identifier '=' tok_int_constant TypeAnnotations CommaOrSemicolonOptional + CaptureDocText EnumValue TypeAnnotations CommaOrSemicolonOptional { - pdebug("EnumDef -> tok_identifier = tok_int_constant"); - if ($4 < 0) { - pwarning(1, "Negative value supplied for enum %s.\n", $2); - } - if ($4 > INT_MAX) { - pwarning(1, "64-bit value supplied for enum %s.\n", $2); - } - validate_simple_identifier( $2); - $$ = new t_enum_value($2, static_cast($4)); + pdebug("EnumDef -> EnumValue"); + $$ = $2; if ($1 != NULL) { $$->set_doc($1); } - if ($5 != NULL) { - $$->annotations_ = $5->annotations_; - delete $5; - } - } -| - CaptureDocText tok_identifier TypeAnnotations CommaOrSemicolonOptional - { - pdebug("EnumDef -> tok_identifier"); - validate_simple_identifier( $2); - $$ = new t_enum_value($2); - if ($1 != NULL) { - $$->set_doc($1); + if (g_parse_mode == PROGRAM) { + // The scope constants never get deleted, so it's okay for us + // to share a single t_const object between our scope and the parent + // scope + t_const* constant = new t_const(g_type_i32, $2->get_name(), + new t_const_value($2->get_value())); + g_scope->add_constant($2->get_name(), constant); + if (g_parent_scope != NULL) { + g_parent_scope->add_constant(g_parent_prefix + $2->get_name(), + constant); + } } if ($3 != NULL) { $$->annotations_ = $3->annotations_; @@ -634,6 +634,33 @@ EnumDef: } } +EnumValue: + tok_identifier '=' tok_int_constant + { + pdebug("EnumValue -> tok_identifier = tok_int_constant"); + if ($3 < INT32_MIN || $3 > INT32_MAX) { + // Note: this used to be just a warning. However, since thrift always + // treats enums as i32 values, I'm changing it to a fatal error. + // I doubt this will affect many people, but users who run into this + // will have to update their thrift files to manually specify the + // truncated i32 value that thrift has always been using anyway. + failure("64-bit value supplied for enum %s will be truncated.", $1); + } + y_enum_val = static_cast($3); + $$ = new t_enum_value($1, y_enum_val); + } + | + tok_identifier + { + pdebug("EnumValue -> tok_identifier"); + validate_simple_identifier( $1); + if (y_enum_val == INT32_MAX) { + failure("enum value overflow at enum %s", $1); + } + ++y_enum_val; + $$ = new t_enum_value($1, y_enum_val); + } + Senum: tok_senum tok_identifier '{' SenumDefList '}' TypeAnnotations { -- cgit v1.2.1