On Tue, Dec 7, 2021 at 6:07 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> 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.
>
LGTM.
------
Kind Regards,
Peter Smith.
Fujitsu Australia