Re: parse_subscription_options - suggested improvements

On 12/5/21, 9:21 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> 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.

I'll admit I was surprised that my compiler didn't complain about
this, but I wouldn't be surprised at all if others did.  I agree that
there is no strong need to remove the initializations from the calling
functions.

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

Attached a v14 with the initializations added back.

Nathan