Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/624#issuecomment-143038915
I really like the general idea. These endless boolean args lists deserve to
be eliminated, that's for sure.
But why do we replace them by named constants/enum values? Is this
generate_deserialize_field(out, &fkey, true /* declare */, "",
NOT_IN_CLASS, NO_COERCE_DATA, NOT_IN_KEY);
generate_deserialize_field(out, &fval, true /* declare */, "",
NOT_IN_CLASS, NO_COERCE_DATA, NOT_IN_KEY, IN_CONTAINER);
really better than this?
generate_deserialize_field(out, &fkey, true, "", false, false, true);
generate_deserialize_field(out, &fval, true, "", false, false, false,
true);
Why not do this instead?
void generate_deserialize_field(ofstream& out, t_field* tfield, string
prefix, FLAGS flags) {
...
}
and call it like
generate_deserialize_field(out, &fval, "", DECLARE_FIELD |
IN_CONTAINER);
generate_deserialize_field(out, &fval, "", DECLARE_FIELD | IN_CLASS |
COERCE_DATA | IN_KEY | IN_CONTAINER);
The second example is a bit constructed, but you get the idea. All
```NO_XXX```s can be omitted because they are ```0```, therefore only the flags
set need to be specified. I agree that there would be two drawbacks: one is
the limitation to 32 (maybe 64) bits, which at least has the potential to
become an issue later on; the other is that we lose type safety again - but we
only have one ```flags``` argument, hence not many chances to mix things up.
Another approach I frequently use is passing a struct containing all the
args. These can be allocated on the stack and are usually quite cheap. The
called routine gets a const ref or ptr->const which is also cheap.
Last not least ... would be awesome to have a similar solution in all the
other generators as well. Functions with more than five args are a code smell,
and we have plenty of them.
As always, other opinions are welcome.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
