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.

7 files changed, 62 insertions, 74 deletions

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<t_enum_value*>& enum_values = get_constants();
-    std::vector<t_enum_value*>::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<t_enum_value*> 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<std::string, std::string> 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<tenum>     Enum
 %type<tenum>     EnumDefList
 %type<tenumv>    EnumDef
+%type<tenumv>    EnumValue
 
 %type<ttypedef>  Senum
 %type<tbase>     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<t_enum_value*>& 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<int>($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<int32_t>($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
     {