[GitHub] [thrift] KarboniteKream commented on a diff in pull request #2765: THRIFT-4086: Use true type when generating field meta data

["KarboniteKream (via GitHub)" <gi...@apache.org>]

KarboniteKream commented on code in PR #2765:
URL: https://github.com/apache/thrift/pull/2765#discussion_r1161114230


##########
compiler/cpp/src/thrift/generate/t_oop_generator.h:
##########
@@ -70,7 +70,7 @@ class t_oop_generator : public t_generator {
   }
 
   virtual void generate_java_doc(std::ostream& out, t_field* field) {
-    if (field->get_type()->is_enum()) {
+    if (get_true_type(field->get_type())->is_enum()) {

Review Comment:
   Sure thing! This `@see` is already appended to some docs in files for `TestDefinitionOrder` (it's how I found I needed to change this line), but I'll add some more docs to `.thrift` files to confirm it's appended correctly.
   
   What did you have in mind for asserting the result? I can't think of anything else other than checking whether the generate file contains a `/* ... @see ... */` string. 🤔 



##########
compiler/cpp/src/thrift/generate/t_java_generator.cc:
##########
@@ -3023,46 +3023,46 @@ void t_java_generator::generate_metadata_for_field_annotations(std::ostream& out
 }
 
 void t_java_generator::generate_field_value_meta_data(std::ostream& out, t_type* type) {
+  t_type* ttype = get_true_type(type);
   out << endl;
   indent_up();
   indent_up();
-  t_type* ttype = get_true_type(type);
   if (ttype->is_struct() || ttype->is_xception()) {
     indent(out) << "new "
                    "org.apache.thrift.meta_data.StructMetaData(org.apache.thrift.protocol.TType."
                    "STRUCT, "
-                << type_name(type) << ".class";
-  } else if (type->is_container()) {
-    if (type->is_list()) {
+                << type_name(ttype) << ".class";
+  } else if (ttype->is_container()) {
+    if (ttype->is_list()) {
       indent(out)
           << "new org.apache.thrift.meta_data.ListMetaData(org.apache.thrift.protocol.TType.LIST, ";
-      t_type* elem_type = ((t_list*)type)->get_elem_type();
+      t_type* elem_type = ((t_list*)ttype)->get_elem_type();
       generate_field_value_meta_data(out, elem_type);
-    } else if (type->is_set()) {
+    } else if (ttype->is_set()) {
       indent(out)
           << "new org.apache.thrift.meta_data.SetMetaData(org.apache.thrift.protocol.TType.SET, ";
-      t_type* elem_type = ((t_set*)type)->get_elem_type();
+      t_type* elem_type = ((t_set*)ttype)->get_elem_type();
       generate_field_value_meta_data(out, elem_type);
     } else { // map
       indent(out)
           << "new org.apache.thrift.meta_data.MapMetaData(org.apache.thrift.protocol.TType.MAP, ";
-      t_type* key_type = ((t_map*)type)->get_key_type();
-      t_type* val_type = ((t_map*)type)->get_val_type();
+      t_type* key_type = ((t_map*)ttype)->get_key_type();
+      t_type* val_type = ((t_map*)ttype)->get_val_type();
       generate_field_value_meta_data(out, key_type);
       out << ", ";
       generate_field_value_meta_data(out, val_type);
     }
-  } else if (type->is_enum()) {
+  } else if (ttype->is_enum()) {
     indent(out)
         << "new org.apache.thrift.meta_data.EnumMetaData(org.apache.thrift.protocol.TType.ENUM, "
-        << type_name(type) << ".class";
+        << type_name(ttype) << ".class";
   } else {
     indent(out) << "new org.apache.thrift.meta_data.FieldValueMetaData("
-                << get_java_type_string(type);
-    if (type->is_typedef()) {
-      indent(out) << ", \"" << ((t_typedef*)type)->get_symbolic() << "\"";
-    } else if (type->is_binary()) {
+                << get_java_type_string(ttype);
+    if (ttype->is_binary()) {
       indent(out) << ", true";
+    } else if (type->is_typedef()) {
+      indent(out) << ", \"" << ((t_typedef*)type)->get_symbolic() << "\"";
     }

Review Comment:
   Something like `Cannot resolve type of field ..., defaulting to typedef`? Let me test this on our project's Thrift files to see if the warning message would would be correct.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org