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.
diff --git a/compiler/cpp/src/generate/t_c_glib_generator.cc b/compiler/cpp/src/generate/t_c_glib_generator.cc
index 96ce99e..c726b0c 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 @@
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 85eeeef..f019ba8 100755
--- a/compiler/cpp/src/generate/t_cpp_generator.cc
+++ b/compiler/cpp/src/generate/t_cpp_generator.cc
@@ -524,7 +524,7 @@
}
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 706506a..ecf908b 100644
--- a/compiler/cpp/src/generate/t_d_generator.cc
+++ b/compiler/cpp/src/generate/t_d_generator.cc
@@ -188,9 +188,7 @@
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 bcd7e5e..9a3e909 100644
--- a/compiler/cpp/src/generate/t_go_generator.cc
+++ b/compiler/cpp/src/generate/t_go_generator.cc
@@ -829,11 +829,7 @@
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 ec847d1..94cb26e 100644
--- a/compiler/cpp/src/parse/t_enum.h
+++ b/compiler/cpp/src/parse/t_enum.h
@@ -76,10 +76,9 @@
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 @@
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 @@
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 3a4a90a..26798d7 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 da5c562..3755d13 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 @@
%type<tenum> Enum
%type<tenum> EnumDefList
%type<tenumv> EnumDef
+%type<tenumv> EnumValue
%type<ttypedef> Senum
%type<tbase> SenumDefList
@@ -569,7 +577,7 @@
$$->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 @@
{
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 @@
}
}
+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
{
diff --git a/lib/cpp/test/EnumTest.cpp b/lib/cpp/test/EnumTest.cpp
new file mode 100644
index 0000000..21bf819
--- /dev/null
+++ b/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()
diff --git a/lib/cpp/test/Makefile.am b/lib/cpp/test/Makefile.am
index c1fad3e..e68e66b 100755
--- a/lib/cpp/test/Makefile.am
+++ b/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 @@
TFileTransportTest \
UnitTests \
link_test \
- OpenSSLManualInitTest
+ OpenSSLManualInitTest \
+ EnumTest
# disable these test ... too strong
# processor_test
# concurrency_test
@@ -112,6 +115,13 @@
-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 @@
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 $<
diff --git a/test/EnumTest.thrift b/test/EnumTest.thrift
new file mode 100644
index 0000000..e6376f5
--- /dev/null
+++ b/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
+}
+