You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "KarboniteKream (via GitHub)" <gi...@apache.org> on 2023/03/05 12:46:57 UTC
[GitHub] [thrift] KarboniteKream opened a new pull request, #2765: THRIFT-4086: Use true type when generating field meta data
KarboniteKream opened a new pull request, #2765:
URL: https://github.com/apache/thrift/pull/2765
Client: java
This PR is a follow-up to #2619, and applies the same change to all other types. This way, the generated field meta data is the same for all types, regardless of the order of definitions in the `.thrift` file.
The main motivation for this PR was to extend the change in the previous PR to enums. Then in order to keep the behavior consistent across all types, I applied it to other types. I've extended the tests to cover all cases.
However, there is one behavioral change when it comes to typedefs. This PR and #2619 use `get_true_type()`, which means that whenever the field definition uses a typedef'd struct or enum (for example `typedef MyType MyTypedef`), the generated metadata will not use `FieldValueMetaData(..., "MyTypedef")`, but `FieldValueMetaData(..., MyType.class)` instead. But when we use something like `typedef i8 MyInt`, we generate `FieldValueMetaData(..., "MyInt")`. Is this okay?
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] KarboniteKream commented on pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "KarboniteKream (via GitHub)" <gi...@apache.org>.
KarboniteKream commented on PR #2765:
URL: https://github.com/apache/thrift/pull/2765#issuecomment-1455087458
There seem to be unrelated failures on AppVeyor:
```
C:\projects\thrift\lib\cpp\test\SecurityTest.cpp(223): error C2065: 'OPENSSL_VERSION_MAJOR': undeclared identifier [C:\projects\build\MSVC2017\x64\lib\cpp\test\SecurityTest.vcxproj]
```
--
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
[GitHub] [thrift] Jimexist commented on a diff in pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on code in PR #2765:
URL: https://github.com/apache/thrift/pull/2765#discussion_r1161087464
##########
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:
can you add a doc to test?
--
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
[GitHub] [thrift] KarboniteKream commented on a diff in pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "KarboniteKream (via GitHub)" <gi...@apache.org>.
KarboniteKream commented on code in PR #2765:
URL: https://github.com/apache/thrift/pull/2765#discussion_r1161114593
##########
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 %s, using typedef`? Let me test this on our project's Thrift files to see if the warning message 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
[GitHub] [thrift] Jimexist commented on a diff in pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on code in PR #2765:
URL: https://github.com/apache/thrift/pull/2765#discussion_r1161087390
##########
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:
maybe add warning here?
--
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
[GitHub] [thrift] KarboniteKream commented on a diff in pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "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
[GitHub] [thrift] KarboniteKream commented on a diff in pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "KarboniteKream (via GitHub)" <gi...@apache.org>.
KarboniteKream commented on code in PR #2765:
URL: https://github.com/apache/thrift/pull/2765#discussion_r1161114593
##########
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 %s, using 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
[GitHub] [thrift] Jimexist merged pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist merged PR #2765:
URL: https://github.com/apache/thrift/pull/2765
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] KarboniteKream commented on a diff in pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "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 generated file contains a `/* ... @see ... */` string. 🤔
--
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
[GitHub] [thrift] KarboniteKream commented on pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "KarboniteKream (via GitHub)" <gi...@apache.org>.
KarboniteKream commented on PR #2765:
URL: https://github.com/apache/thrift/pull/2765#issuecomment-1514139742
Rebased on `master` to fix failing Go tests
--
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
[GitHub] [thrift] KarboniteKream commented on a diff in pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "KarboniteKream (via GitHub)" <gi...@apache.org>.
KarboniteKream commented on code in PR #2765:
URL: https://github.com/apache/thrift/pull/2765#discussion_r1161114593
##########
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 %s, using typedef`? Let me test this on our project's Thrift files to see if the warning message 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
[GitHub] [thrift] KarboniteKream commented on a diff in pull request #2765: THRIFT-4086: Use true type when generating field meta data
Posted by "KarboniteKream (via GitHub)" <gi...@apache.org>.
KarboniteKream commented on code in PR #2765:
URL: https://github.com/apache/thrift/pull/2765#discussion_r1170176549
##########
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:
I've double-checked and I think a warning is not suitable here. This `else` handles typedefs of primitive types (for example `typedef i64 Timestamp`), so it can generate `FieldValueMetaData(TType.I64, "Timestamp")`, which is the intended behavior.
This PR merely changes the evaluation order, to prioritize `FieldValueMetaData(TType.BINARY, true)` over typedef metadata definition, since it's now using `get_java_type_string(ttype)` as the type.
--
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