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.

git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@892358 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/compiler/cpp/src/generate/t_java_generator.cc b/compiler/cpp/src/generate/t_java_generator.cc
index 1282e2d..09d2fd1 100644 (file)
--- a/compiler/cpp/src/generate/t_java_generator.cc
+++ b/compiler/cpp/src/generate/t_java_generator.cc
@@ -367,11 +367,11 @@ void t_java_generator::generate_enum(t_enum* tenum) {
   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 @@ void t_java_generator::generate_enum(t_enum* tenum) {
     }
 
     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 @@ void t_java_generator::print_const_value(std::ofstream& out, string name, t_type
     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 @@ string t_java_generator::render_const_value(ofstream& out, string name, t_type*
       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);

diff --git a/compiler/cpp/src/main.cc b/compiler/cpp/src/main.cc
index 7a5d2d4..7eb4801 100644 (file)
--- a/compiler/cpp/src/main.cc
+++ b/compiler/cpp/src/main.cc
@@ -702,9 +702,24 @@ void validate_const_rec(std::string name, t_type* type, t_const_value* value) {
       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";

diff --git a/compiler/cpp/src/parse/t_const_value.h b/compiler/cpp/src/parse/t_const_value.h
index a7d6e31..5bfaeb2 100644 (file)
--- a/compiler/cpp/src/parse/t_const_value.h
+++ b/compiler/cpp/src/parse/t_const_value.h
@@ -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 @@ class t_const_value {
     CV_DOUBLE,
     CV_STRING,
     CV_MAP,
-    CV_LIST
+    CV_LIST,
+    CV_IDENTIFIER
   };
 
   t_const_value() {}
@@ -66,7 +67,15 @@ class t_const_value {
   }
 
   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 @@ class t_const_value {
     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 @@ class t_const_value {
   std::string stringVal_;
   int64_t intVal_;
   double doubleVal_;
+  std::string identifierVal_;
+  t_enum* enum_;
 
   t_const_value_type valType_;
 

diff --git a/compiler/cpp/src/parse/t_enum.h b/compiler/cpp/src/parse/t_enum.h
index 740f95c..45d1606 100644 (file)
--- a/compiler/cpp/src/parse/t_enum.h
+++ b/compiler/cpp/src/parse/t_enum.h
@@ -44,6 +44,28 @@ class t_enum : public t_type {
     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 @@ 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 68e905b..283a87e 100644 (file)
--- a/compiler/cpp/src/parse/t_enum_value.h
+++ b/compiler/cpp/src/parse/t_enum_value.h
@@ -55,6 +55,11 @@ class t_enum_value : public t_doc {
     return value_;
   }
 
+  void set_value(int val) {
+    has_value_ = true;
+    value_ = val;
+  }
+
  private:
   std::string name_;
   bool has_value_;

diff --git a/compiler/cpp/src/parse/t_scope.h b/compiler/cpp/src/parse/t_scope.h
index 122e325..d3f9d26 100644 (file)
--- a/compiler/cpp/src/parse/t_scope.h
+++ b/compiler/cpp/src/parse/t_scope.h
@@ -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 @@ class t_scope {
     }
   }
 
+  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

diff --git a/compiler/cpp/src/parse/t_struct.h b/compiler/cpp/src/parse/t_struct.h
index 76c24f2..aeee112 100644 (file)
--- a/compiler/cpp/src/parse/t_struct.h
+++ b/compiler/cpp/src/parse/t_struct.h
@@ -123,6 +123,16 @@ class t_struct : public t_type {
     }
   }
 
+  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_;

diff --git a/compiler/cpp/src/thrifty.yy b/compiler/cpp/src/thrifty.yy
index 987dd8d..0fc90da 100644 (file)
--- a/compiler/cpp/src/thrifty.yy
+++ b/compiler/cpp/src/thrifty.yy
@@ -504,6 +504,7 @@ Enum:
       pdebug("Enum -> tok_enum tok_identifier { EnumDefList }");
       $$ = $4;
       $$->set_name($2);
+      $$->resolve_values();
     }
 
 EnumDefList:
@@ -583,6 +584,7 @@ Const:
     {
       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 @@ Const:
         if (g_parent_scope != NULL) {
           g_parent_scope->add_constant(g_parent_prefix + $3, $$);
         }
-
       } else {
         $$ = NULL;
       }
@@ -620,15 +621,8 @@ ConstValue:
 | 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 @@ Field:
       $$ = 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);
       }

diff --git a/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java b/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java
index 5d1c3cb..04ef77f 100755 (executable)
--- a/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java
+++ b/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java
@@ -380,6 +380,11 @@ public class TCompactProtocolTest {
         // TODO Auto-generated method stub
         
       }
+
+      public void methodWithDefaultArgs(int something) throws TException {
+        // TODO Auto-generated method stub
+        
+      }
     };
     
     Srv.Processor testProcessor = new Srv.Processor(handler);

diff --git a/test/DebugProtoTest.thrift b/test/DebugProtoTest.thrift
index 549462e..6a9c785 100644 (file)
--- a/test/DebugProtoTest.thrift
+++ b/test/DebugProtoTest.thrift
@@ -214,6 +214,7 @@ const CompactProtoTestStruct COMPACT_TEST = {
 }
 
 
+const i32 MYCONST = 2
 
 service Srv {
   i32 Janky(1: i32 arg);
@@ -223,6 +224,8 @@ service Srv {
   void voidMethod();
   i32 primitiveMethod();
   CompactProtoTestStruct structMethod();
+  
+  void methodWithDefaultArgs(1: i32 something = MYCONST);
 }
 
 service Inherited extends Srv {
@@ -253,8 +256,25 @@ service ReverseOrderService {
 }
 
 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 {