allengeorge commented on a change in pull request #2310:
URL: https://github.com/apache/thrift/pull/2310#discussion_r579902112
##########
File path: compiler/cpp/src/thrift/generate/t_rs_generator.cc
##########
@@ -1141,13 +1141,18 @@ void t_rs_generator::render_struct_impl(
render_struct_constructor(struct_name, tstruct, struct_type);
}
- render_struct_sync_read(struct_name, tstruct, struct_type);
- render_struct_sync_write(tstruct, struct_type);
-
if (struct_type == t_rs_generator::T_RESULT) {
render_result_struct_to_result_method(tstruct);
}
+ f_gen_ << "}" << endl;
+ f_gen_ << endl;
+
+ f_gen_ << "impl TObject for " << struct_name << " {" << endl;
+
Review comment:
Please fix indentation here. You need to add an `indent_up` before
rendering the read/write methods.
##########
File path: compiler/cpp/src/thrift/generate/t_rs_generator.cc
##########
@@ -907,16 +907,16 @@ void t_rs_generator::render_enum_impl(const string&
enum_name) {
// taking a reference to the enum
f_gen_
<< indent()
- << "pub fn write_to_out_protocol(self, o_prot: &mut dyn TOutputProtocol)
-> thrift::Result<()> {"
+ << "fn write_to_out_protocol(&self, o_prot: &mut dyn TOutputProtocol) ->
thrift::Result<()> {"
Review comment:
Please remove the comment on line 984. As a result of your change the
comment is no longer valid.
##########
File path: compiler/cpp/src/thrift/generate/t_rs_generator.cc
##########
@@ -898,7 +898,7 @@ void t_rs_generator::render_enum_definition(t_enum* tenum,
const string& enum_na
}
void t_rs_generator::render_enum_impl(const string& enum_name) {
- f_gen_ << "impl " << enum_name << " {" << endl;
+ f_gen_ << "impl TObject for " << enum_name << " {" << endl;
Review comment:
Please rename to `TSerializable`.
##########
File path: compiler/cpp/src/thrift/generate/t_rs_generator.cc
##########
@@ -1141,13 +1141,18 @@ void t_rs_generator::render_struct_impl(
render_struct_constructor(struct_name, tstruct, struct_type);
}
- render_struct_sync_read(struct_name, tstruct, struct_type);
- render_struct_sync_write(tstruct, struct_type);
-
if (struct_type == t_rs_generator::T_RESULT) {
render_result_struct_to_result_method(tstruct);
}
+ f_gen_ << "}" << endl;
Review comment:
Please fix indentation here. You need to add an `indent_down()` call
before closing the `impl` block..
##########
File path: compiler/cpp/src/thrift/generate/t_rs_generator.cc
##########
@@ -1406,7 +1411,6 @@ void t_rs_generator::render_struct_sync_write(
) {
f_gen_
<< indent()
- << visibility_qualifier(struct_type)
Review comment:
I assume the reason why this can be removed is because the methods on
the trait are already in the public scope?
##########
File path: compiler/cpp/src/thrift/generate/t_rs_generator.cc
##########
@@ -1406,7 +1411,6 @@ void t_rs_generator::render_struct_sync_write(
) {
f_gen_
<< indent()
- << visibility_qualifier(struct_type)
Review comment:
Also, please note that as a result of this _all_ structs (args and
results included) suddenly have public read/write methods. This was not
intended originally, and it's not clear we want it as a result of this PR as
well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
