Re: [PATCH v2] commit-tree: utilize parse-options api

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 1, 2019, 20:53 UTC
Message-ID: <CAPig+cQoZQCTAzaDiaAdAvSqHBHSoapDoVLjPtpKjCEVSBL57g@mail.gmail.com>
In-Reply-To: <20190301190954.GG30847@sigill.intra.peff.net>

On Fri, Mar 1, 2019 at 2:10 PM Jeff King <peff@peff.net> 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...

Show 11 quoted lines

> +/*
> + * 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.