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

@@ -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();

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();
     }
   }

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;

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());

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_;

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_;
 };
 

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
     {

lib/cpp/test/EnumTest.cpp

@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#define BOOST_TEST_MODULE EnumTest
+#include <boost/test/unit_test.hpp>
+#include "thrift/test/gen-cpp/EnumTest_types.h"
+
+BOOST_AUTO_TEST_SUITE( EnumTest )
+
+BOOST_AUTO_TEST_CASE( test_enum ) {
+  // Check that all the enum values match what we expect
+  BOOST_CHECK_EQUAL(ME1_0, 0);
+  BOOST_CHECK_EQUAL(ME1_1, 1);
+  BOOST_CHECK_EQUAL(ME1_2, 2);
+  BOOST_CHECK_EQUAL(ME1_3, 3);
+  BOOST_CHECK_EQUAL(ME1_5, 5);
+  BOOST_CHECK_EQUAL(ME1_6, 6);
+
+  BOOST_CHECK_EQUAL(ME2_0, 0);
+  BOOST_CHECK_EQUAL(ME2_1, 1);
+  BOOST_CHECK_EQUAL(ME2_2, 2);
+
+  BOOST_CHECK_EQUAL(ME3_0, 0);
+  BOOST_CHECK_EQUAL(ME3_1, 1);
+  BOOST_CHECK_EQUAL(ME3_N2, -2);
+  BOOST_CHECK_EQUAL(ME3_N1, -1);
+  BOOST_CHECK_EQUAL(ME3_D0, 0);
+  BOOST_CHECK_EQUAL(ME3_D1, 1);
+  BOOST_CHECK_EQUAL(ME3_9, 9);
+  BOOST_CHECK_EQUAL(ME3_10, 10);
+
+  BOOST_CHECK_EQUAL(ME4_A, 0x7ffffffd);
+  BOOST_CHECK_EQUAL(ME4_B, 0x7ffffffe);
+  BOOST_CHECK_EQUAL(ME4_C, 0x7fffffff);
+}
+
+BOOST_AUTO_TEST_CASE( test_enum_constant ) {
+  MyStruct ms;
+  BOOST_CHECK_EQUAL(ms.me2_2, 2);
+  BOOST_CHECK_EQUAL(ms.me3_n2, -2);
+  BOOST_CHECK_EQUAL(ms.me3_d1, 1);
+}
+
+BOOST_AUTO_TEST_SUITE_END()

lib/cpp/test/Makefile.am

@@ -20,6 +20,8 @@
 noinst_LTLIBRARIES = libtestgencpp.la libprocessortest.la
 nodist_libtestgencpp_la_SOURCES = \
 	gen-cpp/DebugProtoTest_types.cpp \
+	gen-cpp/EnumTest_types.cpp \
+	gen-cpp/EnumTest_types.h \
 	gen-cpp/OptionalRequiredTest_types.cpp \
 	gen-cpp/DebugProtoTest_types.cpp \
 	gen-cpp/ThriftTest_types.cpp \
@@ -65,7 +67,8 @@ check_PROGRAMS = \
 	TFileTransportTest \
 	UnitTests \
 	link_test \
-	OpenSSLManualInitTest
+	OpenSSLManualInitTest \
+	EnumTest
 # disable these test ... too strong
 #       processor_test
 #	concurrency_test
@@ -112,6 +115,13 @@ ZlibTest_LDADD = \
   -l:libboost_unit_test_framework.a \
   -lz
 
+EnumTest_SOURCES = \
+  EnumTest.cpp
+
+EnumTest_LDADD = \
+  libtestgencpp.la \
+  $(BOOST_ROOT_PATH)/lib/libboost_unit_test_framework.a
+
 TFileTransportTest_SOURCES = \
 	TFileTransportTest.cpp
 
@@ -233,6 +243,9 @@ THRIFT = $(top_builddir)/compiler/cpp/thrift
 gen-cpp/DebugProtoTest_types.cpp gen-cpp/DebugProtoTest_types.h: $(top_srcdir)/test/DebugProtoTest.thrift
 	$(THRIFT) --gen cpp:dense $<
 
+gen-cpp/EnumTest_types.cpp gen-cpp/EnumTest_types.h: $(top_srcdir)/test/EnumTest.thrift
+	$(THRIFT) --gen cpp $<
+
 gen-cpp/OptionalRequiredTest_types.cpp gen-cpp/OptionalRequiredTest_types.h: $(top_srcdir)/test/OptionalRequiredTest.thrift
 	$(THRIFT) --gen cpp:dense $<
 

test/EnumTest.thrift

@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ * Contains some contributions under the Thrift Software License.
+ * Please see doc/old-thrift-license.txt in the Thrift distribution for
+ * details.
+ */
+
+enum MyEnum1 {
+  ME1_0 = 0,
+  ME1_1 = 1,
+  ME1_2,
+  ME1_3,
+  ME1_5 = 5,
+  ME1_6,
+}
+
+enum MyEnum2 {
+  ME2_0,
+  ME2_1,
+  ME2_2,
+}
+
+enum MyEnum3 {
+  ME3_0,
+  ME3_1,
+  ME3_N2 = -2,
+  ME3_N1,
+  ME3_D0,
+  ME3_D1,
+  ME3_9 = 9,
+  ME3_10,
+}
+
+enum MyEnum4 {
+  ME4_A = 0x7ffffffd
+  ME4_B
+  ME4_C
+  // attempting to define another enum value here fails
+  // with an overflow error, as we overflow values that can be
+  // represented with an i32.
+}
+
+enum MyEnum5 {
+  // attempting to explicitly use values out of the i32 range will also fail
+  // ME5_A = 0x80000000,
+  // ME5_B = 0x100000000,
+}
+
+struct MyStruct {
+  1: MyEnum2 me2_2 = MyEnum1.ME2_2
+  2: MyEnum3 me3_n2 = MyEnum3.ME3_N2
+  3: MyEnum3 me3_d1 = MyEnum3.ME3_D1
+}
+