diff --git a/compiler/cpp/src/generate/t_c_glib_generator.cc b/compiler/cpp/src/generate/t_c_glib_generator.cc index 373e6a8e280..2b02f3a68a9 100644 --- a/compiler/cpp/src/generate/t_c_glib_generator.cc +++ b/compiler/cpp/src/generate/t_c_glib_generator.cc @@ -307,10 +307,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 298096d3265..a06bc7b63e7 100755 --- a/compiler/cpp/src/generate/t_cpp_generator.cc +++ b/compiler/cpp/src/generate/t_cpp_generator.cc @@ -507,7 +507,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 58dbb9aae39..046e124a3da 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 a216c2bed12..ce3f5751685 100644 --- a/compiler/cpp/src/generate/t_go_generator.cc +++ b/compiler/cpp/src/generate/t_go_generator.cc @@ -686,11 +686,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 45d160665fc..1a0f23c88da 100644 --- a/compiler/cpp/src/parse/t_enum.h +++ b/compiler/cpp/src/parse/t_enum.h @@ -74,18 +74,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 3a4a90a8319..26798d7333c 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 7c4a90bbc06..8effb3a9527 100755 --- 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; @@ -197,6 +204,7 @@ const int struct_is_union = 1; %type Enum %type EnumDefList %type EnumDef +%type EnumValue %type Senum %type SenumDefList @@ -566,7 +574,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(); @@ -594,36 +602,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_; @@ -631,6 +631,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 = $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 00000000000..21bf81956f7 --- /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 +#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 d04cfb501c1..8c043ea0897 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 \ @@ -60,7 +62,8 @@ check_PROGRAMS = \ TransportTest \ ZlibTest \ TFileTransportTest \ - UnitTests + UnitTests \ + EnumTest # disable these test ... too strong # processor_test # concurrency_test @@ -106,6 +109,13 @@ ZlibTest_LDADD = \ $(BOOST_ROOT_PATH)/lib/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 @@ -206,6 +216,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 $< diff --git a/test/EnumTest.thrift b/test/EnumTest.thrift new file mode 100644 index 00000000000..2ecfe9853f0 --- /dev/null +++ b/test/EnumTest.thrift @@ -0,0 +1,46 @@ +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 +}