Re: parse_subscription_options - suggested improvements

On Mon, Dec 06, 2021 at 11:28:12AM +1100, Peter Smith wrote:
> For the initialization of opts I put memset within the function to
> make it explicit that the bit-masks will work as intended without
> having to look back at calling code for the initial values. In any
> case, I think the caller declarations of SubOpts are trivial, (e.g.
> SubOpts opts = {0};) so I felt caller initializations don't need to be
> changed regardless of the memset.

It seems to me that not initializing these may cause some compilation
warnings. memset(0) at the beginning of parse_subscription_options()
is an improvement.

> My patch was meant only to remove all the redundant conditions of the
> HEAD code, so I did not rearrange any of the logic at all. Personally,
> I also think your v13 is better and easier to read, but those subtle
> behaviour differences were something I'd deliberately avoided in v12.
> However, if the committer thinks it does not matter then your v13 is
> fine by me.

Well, there is always the argument that it could be confusing as a
different combination of options generates a slightly-different error,
but the user would get warned about each one of his/her mistakes at
the end, so the result is the same.

- if (opts->enabled &&
- IsSet(supported_opts, SUBOPT_ENABLED) &&
- !IsSet(opts->specified_opts, SUBOPT_ENABLED))
+ if (opts->enabled)

I see. The last condition on the specified options in the last two
checks is removed thanks to the first two checks. As a matter of
consistency with those error strings, keeping each !IsSet() would be
cleaner. But I agree that v13 is better than that, without removing
the two initializations.
--
Michael