On Wed, Jul 7, 2021 at 7:36 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Jul-07, Peter Smith wrote:
>
> > 1. Zap 'opts' up-front
> >
> > + *
> > + * Caller is expected to have cleared 'opts'.
> >
> > This comment is putting the onus on the caller to "do the right thing".
> >
> > I think that hopeful expectations about input should be removed - the
> > function should just be responsible itself just to zap the SubOpts
> > up-front. It makes the code more readable, and it removes any
> > potential risk of garbage unintentionally passed in that struct.
> >
> > /* Start out with cleared opts. */
> > memset(opts, 0, sizeof(SubOpts));
>
> Yeah, I gave the initialization aspect some thought too when I reviewed
> 0001. Having an explicit memset() just for sanity check is a waste when
> you don't really need it; and we're initializing the struct already at
> declaration time by assigning {0} to it, so having to add memset feels
> like such a waste. Another point in the same area is that some of the
> struct members are initialized to some default value different from 0,
> so I wondered if it would have been better to remove the = {0} and
> instead have another function that would set everything up the way we
> want; but it seemed a bit excessive, so I ended up not suggesting that.
>
> We have many places in the source tree where the caller is expected to
> do the right thing, even when those right things are more complex than
> this one. This one place isn't terribly bad in that regard, given that
> it's a pretty well contained static struct anyway (we would certainly
> not export a struct of this name in any .h file.) I don't think it's
> all that bad.
>
> > Alternatively, at least there should be an assertion for some sanity check.
> >
> > Assert(opt->specified_opts == 0);
>
> No opinion on this. It doesn't seem valuable enough, but maybe I'm on
> the minority on this.
>
I am also not sure if such an assertion adds much value.
> > 2. Remove redundant conditions
> >
> > /* Check for incompatible options from the user. */
> > - if (enabled && *enabled_given && *enabled)
> > + if (opts->enabled &&
> > + IsSet(supported_opts, SUBOPT_ENABLED) &&
> > + IsSet(opts->specified_opts, SUBOPT_ENABLED))
>
> (etc)
>
> Yeah, I thought about this too when I saw the 0002 patch in this series.
> I agree that the extra rechecks are a bit pointless.
>
I don't think the committed patch has made anything worse here or
added any new condition. Now, even if we want to change these
conditions, it is better to discuss them separately. If we see as per
current code these look a bit redundant but OTOH, in the future one
might expect that if the supported option is not passed by the caller
and the user has specified it then it should be an error. For example,
we don't want to support some option via some Alter variant but it is
supported by Create variant.
--
With Regards,
Amit Kapila.
