Skip to content

Alternative implementation of Stringer interface #3106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Alternative implementation of Stringer interface #3106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions compiler/cpp/src/thrift/generate/t_go_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, 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 :)

out << indent() << "return \"" << escape_string(tstruct_name) << "\" + string(v)" << '\n';
} else {
throw "invalid stringer mode: " + stringer_mode_;
}
indent_down();
out << indent() << "}" << '\n' << '\n';

Expand Down Expand Up @@ -4397,4 +4404,6 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n"
" skip_remote\n"
" Skip the generating of -remote folders for the client binaries for services\n")
" Skip the generating of -remote folders for the client binaries for services\n"
" stringer_mode=\n"
" Stringer implementation mode: " + STRINGER_MODE_FMT + ", " + STRINGER_MODE_JSON + ". Default:" + STRINGER_MODE_DEFAULT + "\n")
11 changes: 11 additions & 0 deletions compiler/cpp/src/thrift/generate/t_go_generator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ using std::vector;

const string DEFAULT_THRIFT_IMPORT = "github.com/apache/thrift/lib/go/thrift";
static std::string package_flag;
const string STRINGER_MODE_FMT = "fmt";
const string STRINGER_MODE_JSON = "json";
const string STRINGER_MODE_DEFAULT = STRINGER_MODE_FMT;

/**
* Go code generator.
Expand All @@ -65,6 +68,7 @@ class t_go_generator : public t_generator {
read_write_private_ = false;
ignore_initialisms_ = false;
skip_remote_ = false;
stringer_mode_ = STRINGER_MODE_DEFAULT;
for (iter = parsed_options.begin(); iter != parsed_options.end(); ++iter) {
if (iter->first.compare("package_prefix") == 0) {
gen_package_prefix_ = (iter->second);
Expand All @@ -78,6 +82,12 @@ class t_go_generator : public t_generator {
ignore_initialisms_ = true;
} else if( iter->first.compare("skip_remote") == 0) {
skip_remote_ = true;
} else if (iter->first.compare("stringer_mode") == 0) {
if (iter->second.compare(STRINGER_MODE_FMT) != 0 &&
iter->second.compare(STRINGER_MODE_JSON) != 0) {
throw "bad stringer_mode:" + iter->second;
}
stringer_mode_ = iter->second;
} else {
throw "unknown option go:" + iter->first;
}
Expand Down Expand Up @@ -298,6 +308,7 @@ class t_go_generator : public t_generator {
bool read_write_private_;
bool ignore_initialisms_;
bool skip_remote_;
std::string stringer_mode_;

/**
* File streams
Expand Down
Loading