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.
> 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.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
