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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
