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
signature.asc
Description: PGP signature
