Author: bryanduxbury
Date: Fri Dec 18 19:41:11 2009
New Revision: 892358
URL: http://svn.apache.org/viewvc?rev=892358&view=rev
Log:
THRIFT-632. java: Constants of enum types don't behave well
This patch causes constants of all types to be resolved differently by the
compiler, and makes it so that constants of enum types contain a reference to
the enum type so that code generators can produce the correct names.
Modified:
incubator/thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc
incubator/thrift/trunk/compiler/cpp/src/main.cc
incubator/thrift/trunk/compiler/cpp/src/parse/t_const_value.h
incubator/thrift/trunk/compiler/cpp/src/parse/t_enum.h
incubator/thrift/trunk/compiler/cpp/src/parse/t_enum_value.h
incubator/thrift/trunk/compiler/cpp/src/parse/t_scope.h
incubator/thrift/trunk/compiler/cpp/src/parse/t_struct.h
incubator/thrift/trunk/compiler/cpp/src/thrifty.yy
incubator/thrift/trunk/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java
incubator/thrift/trunk/test/DebugProtoTest.thrift
Modified: incubator/thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc?rev=892358&r1=892357&r2=892358&view=diff
==============================================================================
--- incubator/thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc
(original)
+++ incubator/thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc Fri
Dec 18 19:41:11 2009
@@ -367,11 +367,11 @@
f_enum << string() +
"import java.util.Map;\n" +
"import java.util.HashMap;\n" +
- "import org.apache.thrift.TEnum;" << endl;
+ "import org.apache.thrift.TEnum;" << endl << endl;
generate_java_doc(f_enum, tenum);
indent(f_enum) <<
- "public enum " << tenum->get_name() << " implements TEnum";
+ "public enum " << tenum->get_name() << " implements TEnum ";
scope_up(f_enum);
vector<t_enum_value*> constants = tenum->get_constants();
@@ -392,7 +392,7 @@
}
generate_java_doc(f_enum, *c_iter);
- indent(f_enum) << " " << (*c_iter)->get_name() << "(" << value << ")";
+ indent(f_enum) << (*c_iter)->get_name() << "(" << value << ")";
}
f_enum << ";" << endl << endl;
@@ -487,7 +487,7 @@
string v2 = render_const_value(out, name, type, value);
out << name << " = " << v2 << ";" << endl << endl;
} else if (type->is_enum()) {
- out << name << " = " << value->get_integer() << ";" << endl << endl;
+ out << name << " = " << render_const_value(out, name, type, value) << ";"
<< endl << endl;
} else if (type->is_struct() || type->is_xception()) {
const vector<t_field*>& fields = ((t_struct*)type)->get_members();
vector<t_field*>::const_iterator f_iter;
@@ -602,7 +602,7 @@
throw "compiler error: no const of base type " +
t_base_type::t_base_name(tbase);
}
} else if (type->is_enum()) {
- render << value->get_integer();
+ render << type_name(type, false, false) << "." << value->get_identifier();
} else {
string t = tmp("tmp");
print_const_value(out, t, type, value, true);
Modified: incubator/thrift/trunk/compiler/cpp/src/main.cc
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/compiler/cpp/src/main.cc?rev=892358&r1=892357&r2=892358&view=diff
==============================================================================
--- incubator/thrift/trunk/compiler/cpp/src/main.cc (original)
+++ incubator/thrift/trunk/compiler/cpp/src/main.cc Fri Dec 18 19:41:11 2009
@@ -702,9 +702,24 @@
throw "compiler error: no const of base type " +
t_base_type::t_base_name(tbase) + name;
}
} else if (type->is_enum()) {
- if (value->get_type() != t_const_value::CV_INTEGER) {
+ if (value->get_type() != t_const_value::CV_IDENTIFIER) {
throw "type error: const \"" + name + "\" was declared as enum";
}
+
+ const vector<t_enum_value*>& enum_values =
((t_enum*)type)->get_constants();
+ vector<t_enum_value*>::const_iterator c_iter;
+ bool found = false;
+ for (c_iter = enum_values.begin(); c_iter != enum_values.end(); ++c_iter) {
+ if ((*c_iter)->get_name() == value->get_identifier()) {
+ found = true;
+ break;
+ }
+ }
+ if (!found) {
+ throw "type error: const " + name + " was declared as type "
+ + type->get_name() + " which is an enum, but "
+ + value->get_identifier() + " is not a valid value for that enum";
+ }
} else if (type->is_struct() || type->is_xception()) {
if (value->get_type() != t_const_value::CV_MAP) {
throw "type error: const \"" + name + "\" was declared as
struct/xception";
Modified: incubator/thrift/trunk/compiler/cpp/src/parse/t_const_value.h
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/compiler/cpp/src/parse/t_const_value.h?rev=892358&r1=892357&r2=892358&view=diff
==============================================================================
--- incubator/thrift/trunk/compiler/cpp/src/parse/t_const_value.h (original)
+++ incubator/thrift/trunk/compiler/cpp/src/parse/t_const_value.h Fri Dec 18
19:41:11 2009
@@ -20,7 +20,7 @@
#ifndef T_CONST_VALUE_H
#define T_CONST_VALUE_H
-#include "t_const.h"
+#include "t_enum.h"
#include <stdint.h>
#include <map>
#include <vector>
@@ -38,7 +38,8 @@
CV_DOUBLE,
CV_STRING,
CV_MAP,
- CV_LIST
+ CV_LIST,
+ CV_IDENTIFIER
};
t_const_value() {}
@@ -66,7 +67,15 @@
}
int64_t get_integer() const {
- return intVal_;
+ if (valType_ == CV_IDENTIFIER) {
+ if (enum_ == NULL) {
+ throw "have identifier \"" + get_identifier() + "\", but unset enum on
line!";
+ }
+ t_enum_value* val = enum_->get_constant_by_name(get_identifier());
+ return val->get_value();
+ } else {
+ return intVal_;
+ }
}
void set_double(double val) {
@@ -102,6 +111,19 @@
return listVal_;
}
+ void set_identifier(std::string val) {
+ valType_ = CV_IDENTIFIER;
+ identifierVal_ = val;
+ }
+
+ std::string get_identifier() const {
+ return identifierVal_;
+ }
+
+ void set_enum(t_enum* tenum) {
+ enum_ = tenum;
+ }
+
t_const_value_type get_type() const {
return valType_;
}
@@ -112,6 +134,8 @@
std::string stringVal_;
int64_t intVal_;
double doubleVal_;
+ std::string identifierVal_;
+ t_enum* enum_;
t_const_value_type valType_;
Modified: incubator/thrift/trunk/compiler/cpp/src/parse/t_enum.h
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/compiler/cpp/src/parse/t_enum.h?rev=892358&r1=892357&r2=892358&view=diff
==============================================================================
--- incubator/thrift/trunk/compiler/cpp/src/parse/t_enum.h (original)
+++ incubator/thrift/trunk/compiler/cpp/src/parse/t_enum.h Fri Dec 18 19:41:11
2009
@@ -44,6 +44,28 @@
return constants_;
}
+ t_enum_value* get_constant_by_name(const std::string name) {
+ const std::vector<t_enum_value*>& enum_values = get_constants();
+ std::vector<t_enum_value*>::const_iterator c_iter;
+ for (c_iter = enum_values.begin(); c_iter != enum_values.end(); ++c_iter) {
+ if ((*c_iter)->get_name() == name) {
+ return *c_iter;
+ }
+ }
+ return NULL;
+ }
+
+ t_enum_value* get_constant_by_value(int64_t value) {
+ const std::vector<t_enum_value*>& enum_values = get_constants();
+ std::vector<t_enum_value*>::const_iterator c_iter;
+ for (c_iter = enum_values.begin(); c_iter != enum_values.end(); ++c_iter) {
+ if ((*c_iter)->get_value() == value) {
+ return *c_iter;
+ }
+ }
+ return NULL;
+ }
+
bool is_enum() const {
return true;
}
@@ -52,6 +74,19 @@
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_;
};
Modified: incubator/thrift/trunk/compiler/cpp/src/parse/t_enum_value.h
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/compiler/cpp/src/parse/t_enum_value.h?rev=892358&r1=892357&r2=892358&view=diff
==============================================================================
--- incubator/thrift/trunk/compiler/cpp/src/parse/t_enum_value.h (original)
+++ incubator/thrift/trunk/compiler/cpp/src/parse/t_enum_value.h Fri Dec 18
19:41:11 2009
@@ -55,6 +55,11 @@
return value_;
}
+ void set_value(int val) {
+ has_value_ = true;
+ value_ = val;
+ }
+
private:
std::string name_;
bool has_value_;
Modified: incubator/thrift/trunk/compiler/cpp/src/parse/t_scope.h
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/compiler/cpp/src/parse/t_scope.h?rev=892358&r1=892357&r2=892358&view=diff
==============================================================================
--- incubator/thrift/trunk/compiler/cpp/src/parse/t_scope.h (original)
+++ incubator/thrift/trunk/compiler/cpp/src/parse/t_scope.h Fri Dec 18 19:41:11
2009
@@ -23,8 +23,14 @@
#include <map>
#include <string>
+#include <boost/lexical_cast.hpp>
#include "t_type.h"
#include "t_service.h"
+#include "t_const.h"
+#include "t_const_value.h"
+#include "t_base_type.h"
+#include "t_map.h"
+#include "t_list.h"
/**
* This represents a variable scope used for looking up predefined types and
@@ -70,6 +76,86 @@
}
}
+ void resolve_const_value(t_const_value* const_val, t_type* ttype) {
+ if (ttype->is_map()) {
+ const std::map<t_const_value*, t_const_value*>& map =
const_val->get_map();
+ std::map<t_const_value*, t_const_value*>::const_iterator v_iter;
+ for (v_iter = map.begin(); v_iter != map.end(); ++v_iter) {
+ resolve_const_value(v_iter->first, ((t_map*)ttype)->get_key_type());
+ resolve_const_value(v_iter->second, ((t_map*)ttype)->get_val_type());
+ }
+ } else if (ttype->is_list() || ttype->is_set()) {
+ const std::vector<t_const_value*>& val = const_val->get_list();
+ std::vector<t_const_value*>::const_iterator v_iter;
+ for (v_iter = val.begin(); v_iter != val.end(); ++v_iter) {
+ resolve_const_value((*v_iter), ((t_list*)ttype)->get_elem_type());
+ }
+ } else if (ttype->is_struct()) {
+ t_struct* tstruct = (t_struct*)ttype;
+ const std::map<t_const_value*, t_const_value*>& map =
const_val->get_map();
+ std::map<t_const_value*, t_const_value*>::const_iterator v_iter;
+ for (v_iter = map.begin(); v_iter != map.end(); ++v_iter) {
+ t_field* field =
tstruct->get_field_by_name(v_iter->first->get_string());
+ resolve_const_value(v_iter->second, field->get_type());
+ }
+ } else if (const_val->get_type() == t_const_value::CV_IDENTIFIER) {
+ if (ttype->is_enum()) {
+ const_val->set_enum((t_enum*)ttype);
+ } else {
+ t_const* constant = get_constant(const_val->get_identifier());
+ if (constant == NULL) {
+ throw "No enum value or constant found named \"" +
const_val->get_identifier() + "\"!";
+ }
+
+ if (constant->get_type()->is_base_type()) {
+ switch (((t_base_type*)constant->get_type())->get_base()) {
+ case t_base_type::TYPE_I16:
+ case t_base_type::TYPE_I32:
+ case t_base_type::TYPE_I64:
+ case t_base_type::TYPE_BOOL:
+ case t_base_type::TYPE_BYTE:
+ const_val->set_integer(constant->get_value()->get_integer());
+ break;
+ case t_base_type::TYPE_STRING:
+ const_val->set_string(constant->get_value()->get_string());
+ break;
+ case t_base_type::TYPE_DOUBLE:
+ const_val->set_double(constant->get_value()->get_double());
+ break;
+ case t_base_type::TYPE_VOID:
+ throw "Constants cannot be of type VOID";
+ }
+ } else if (constant->get_type()->is_map()) {
+ const std::map<t_const_value*, t_const_value*>& map =
constant->get_value()->get_map();
+ std::map<t_const_value*, t_const_value*>::const_iterator v_iter;
+
+ const_val->set_map();
+ for (v_iter = map.begin(); v_iter != map.end(); ++v_iter) {
+ const_val->add_map(v_iter->first, v_iter->second);
+ }
+ } else if (constant->get_type()->is_list()) {
+ const std::vector<t_const_value*>& val =
constant->get_value()->get_list();
+ std::vector<t_const_value*>::const_iterator v_iter;
+
+ const_val->set_list();
+ for (v_iter = val.begin(); v_iter != val.end(); ++v_iter) {
+ const_val->add_list(*v_iter);
+ }
+ }
+ }
+ } else if (ttype->is_enum()) {
+ // enum constant with non-identifier value. set the enum and find the
+ // value's name.
+ t_enum* tenum = (t_enum*)ttype;
+ t_enum_value* enum_value =
tenum->get_constant_by_value(const_val->get_integer());
+ if (enum_value == NULL) {
+ throw "Couldn't find a named value in enum " + tenum->get_name() + "
for value " + boost::lexical_cast<std::string>(const_val->get_integer());
+ }
+ const_val->set_identifier(enum_value->get_name());
+ const_val->set_enum(tenum);
+ }
+ }
+
private:
// Map of names to types
Modified: incubator/thrift/trunk/compiler/cpp/src/parse/t_struct.h
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/compiler/cpp/src/parse/t_struct.h?rev=892358&r1=892357&r2=892358&view=diff
==============================================================================
--- incubator/thrift/trunk/compiler/cpp/src/parse/t_struct.h (original)
+++ incubator/thrift/trunk/compiler/cpp/src/parse/t_struct.h Fri Dec 18
19:41:11 2009
@@ -123,6 +123,16 @@
}
}
+ t_field* get_field_by_name(std::string field_name) {
+ members_type::const_iterator m_iter;
+ for (m_iter = members_in_id_order_.begin(); m_iter !=
members_in_id_order_.end(); ++m_iter) {
+ if ((*m_iter)->get_name() == field_name) {
+ return *m_iter;
+ }
+ }
+ return NULL;
+ }
+
private:
members_type members_;
Modified: incubator/thrift/trunk/compiler/cpp/src/thrifty.yy
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/compiler/cpp/src/thrifty.yy?rev=892358&r1=892357&r2=892358&view=diff
==============================================================================
--- incubator/thrift/trunk/compiler/cpp/src/thrifty.yy (original)
+++ incubator/thrift/trunk/compiler/cpp/src/thrifty.yy Fri Dec 18 19:41:11 2009
@@ -504,6 +504,7 @@
pdebug("Enum -> tok_enum tok_identifier { EnumDefList }");
$$ = $4;
$$->set_name($2);
+ $$->resolve_values();
}
EnumDefList:
@@ -583,6 +584,7 @@
{
pdebug("Const -> tok_const FieldType tok_identifier = ConstValue");
if (g_parse_mode == PROGRAM) {
+ g_scope->resolve_const_value($5, $2);
$$ = new t_const($2, $3, $5);
validate_const_type($$);
@@ -590,7 +592,6 @@
if (g_parent_scope != NULL) {
g_parent_scope->add_constant(g_parent_prefix + $3, $$);
}
-
} else {
$$ = NULL;
}
@@ -620,15 +621,8 @@
| tok_identifier
{
pdebug("ConstValue => tok_identifier");
- t_const* constant = g_scope->get_constant($1);
- if (constant != NULL) {
- $$ = constant->get_value();
- } else {
- if (g_parse_mode == PROGRAM) {
- pwarning(1, "Constant strings should be quoted: %s\n", $1);
- }
- $$ = new t_const_value($1);
- }
+ $$ = new t_const_value();
+ $$->set_identifier($1);
}
| ConstList
{
@@ -872,6 +866,7 @@
$$ = new t_field($4, $5, $2);
$$->set_req($3);
if ($6 != NULL) {
+ g_scope->resolve_const_value($6, $4);
validate_field_value($$, $6);
$$->set_value($6);
}
Modified:
incubator/thrift/trunk/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java?rev=892358&r1=892357&r2=892358&view=diff
==============================================================================
---
incubator/thrift/trunk/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java
(original)
+++
incubator/thrift/trunk/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java
Fri Dec 18 19:41:11 2009
@@ -380,6 +380,11 @@
// TODO Auto-generated method stub
}
+
+ public void methodWithDefaultArgs(int something) throws TException {
+ // TODO Auto-generated method stub
+
+ }
};
Srv.Processor testProcessor = new Srv.Processor(handler);
Modified: incubator/thrift/trunk/test/DebugProtoTest.thrift
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/test/DebugProtoTest.thrift?rev=892358&r1=892357&r2=892358&view=diff
==============================================================================
--- incubator/thrift/trunk/test/DebugProtoTest.thrift (original)
+++ incubator/thrift/trunk/test/DebugProtoTest.thrift Fri Dec 18 19:41:11 2009
@@ -214,6 +214,7 @@
}
+const i32 MYCONST = 2
service Srv {
i32 Janky(1: i32 arg);
@@ -223,6 +224,8 @@
void voidMethod();
i32 primitiveMethod();
CompactProtoTestStruct structMethod();
+
+ void methodWithDefaultArgs(1: i32 something = MYCONST);
}
service Inherited extends Srv {
@@ -253,8 +256,25 @@
}
enum SomeEnum {
- ONE
- TWO
+ ONE = 1
+ TWO = 2
+}
+
+const SomeEnum MY_SOME_ENUM = ONE
+
+const SomeEnum MY_SOME_ENUM_1 = 1
+/*const SomeEnum MY_SOME_ENUM_2 = 7*/
+
+const map<SomeEnum,SomeEnum> MY_ENUM_MAP = {
+ ONE : TWO
+}
+
+struct StructWithSomeEnum {
+ 1: SomeEnum blah;
+}
+
+const map<SomeEnum,StructWithSomeEnum> EXTRA_CRAZY_MAP = {
+ ONE : {"blah" : TWO}
}
union TestUnion {
