fishy commented on code in PR #3106:
URL: https://github.com/apache/thrift/pull/3106#discussion_r1977848158
##########
compiler/cpp/src/thrift/generate/t_go_generator.cc:
##########
@@ -1425,8 +1425,15 @@ void
t_go_generator::generate_go_struct_definition(ostream& out,
out << indent() << "return \"<nil>\"" << '\n';
indent_down();
out << indent() << "}" << '\n';
- out << indent() << "return fmt.Sprintf(\"" << escape_string(tstruct_name) <<
"(%+v)\", *p)"
- << '\n';
+ if (stringer_mode_ == STRINGER_MODE_FMT) {
+ out << indent() << "return fmt.Sprintf(\"" << escape_string(tstruct_name)
<< "(%+v)\", *p)"
+ << '\n';
+ } else if (stringer_mode_ == STRINGER_MODE_JSON) {
+ out << indent() << "v, _ := json.Marshal(p)" << '\n';
Review Comment:
in general, we don't want to drop error returns from generated go code. I
would prefer to fallback to the old behavior if we hit an error when doing json
marshaling here.
additionally, this is not the most efficient way to do json marshaling,
especially when the struct is large, as it requires first grow the `[]byte`
used in `json.Marshal` until it can fit the whole string, then it need to be
allocated again when you convert that to `string`. A more efficient way to do
it would be using `json.NewEncoder` with `strings.Builder`, which will avoid
the second allocation, but you need to strip the final `\n` it generates.
we actually already implemented it in
[`SlogTStructWrapper`](https://pkg.go.dev/github.com/apache/thrift/lib/go/thrift#SlogTStructWrapper),
you can check
https://github.com/apache/thrift/blob/v0.21.0/lib/go/thrift/slog.go#L46 for the
implementation. that implementation will only do the json encoding when the
struct is logged via slog. the consideration is that json encoding is much more
heavier than just `fmt.Sprintf`, so we only do that when needed (e.g. when you
need to log it), instead of everywhere the stringer implementation is used (an
example is that for compiler generated code for an exception, its `Error`
implementation also uses stringer implementation under the hood). we definitely
do not want to change default stringer implementation to json as that would
have large performance implications. your approach of adding a compiler arg is
most likely a good compromise, but please take a look at `SlogTStructWrapper`,
and maybe that's already enough for your use-case :)
--
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]
