Use enums instead of multiple boolean arguments

[prashantv]

Multiple boolean arguments in a function call leads to unreadable code.
E.g

generate_deserialize_field(out, &felem, true, "", false, false, false,
  true, true);

vs

generate_deserialize_field(out, &felem, true, "", NOT_IN_CLASS,
  NO_COERCE_DATA, NOT_IN_KEY, IN_CONTAINER, USE_TRUE_TYPE);

The compiler will ensure you cannot pass the wrong type (e.g.
NOT_IN_CLASS where IsOptional is required).

There are no functional changes in this patch.

[Jens-G]

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_XXXs 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.

[prashantv]

Thanks for the feedback - I was thinking about a struct approach, but because it's not easy to create a struct inline, I avoided that.

As you said, flags work, but lose type safety - there's no way to say that COERCE_DATA is not a valid argument for type_to_go_type. I think it's better to have type safety.

If you think the struct approach is better, we can do that, although calls will then probably look like:

DeserializeFieldOpts opts
opts.UseTrueType = true;
opts.InContainer = true;
generate_deserialize_field(out, &felem, true, "", opts);

I think once we decide on an approach, it should be consistently applied for the other compilers. I'm only really working with Go, so I can only really update the Go generator.

[jeking3]

I like this notion, the enums make it more readable. If you would be willing to revive it and reapply it against the current master, we can take another look at it. Also a Jira ticket for the change is needed; see https://thrift.apache.org/docs/HowToContribute

[jeking3]

This pull request is being closed due to lack of activity.
You can re-open it after rebasing on the current master if you would like it to be reconsidered.