On Fri, Mar 1, 2019 at 2:10 PM Jeff King <peff@xxxxxxxx> wrote:
> On Fri, Mar 01, 2019 at 01:13:04PM -0400, Brandon Richardson wrote:
> > + if((!unset) && (!arg)) \
> > + BUG("option callback does not expect negation and requires an argument"); \
Peff didn't highlight this, but compare your use of macro arguments
against his...
> +/*
> + * Similar to the assertions above, but checks that "arg" is always non-NULL.
> + * I.e., that we expect the NOARG and OPTARG flags _not_ to be set. Since
> + * negation is the other common cause of a NULL arg, this also implies
> + * BUG_ON_OPT_NEG(), letting you declare both assertions in a single line.
> + */
> +#define BUG_ON_OPT_NOARG(unset, arg) do { \
> + BUG_ON_OPT_NEG(unset); \
> + if (!(arg)) \
> + BUG("option callback require an argument"); \
> +} while (0)
Note, in particular how Peff used !(arg) rather than (!arg) in your
patch. This distinction is subtle but important enough to warrant
being called out. The reason that Peff did it this way (the _correct_
way) is that, as a macro argument, 'arg' may be a complex expression
rather than a simple boolean. for instance, a caller could conceivably
invoke the macro as:
BUG_ON_OPT_NOARG(unset, foo || bar)
Let's say that 'foo' and 'bar' are both true. With Peff's version,
when the macro is expanded, that expression becomes:
!(true || true)
which evaluates to false as expected and intended. With your version,
it expands to:
(!true || true)
which evaluates to true (since ! has higher precedence than ||), which
is a very different and very unexpected (and likely wrong) result.
