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

From: Jeff King <peff@peff.net>
Date: Feb 28, 2019, 20:56 UTC
Message-ID: <20190228205633.GA12199@sigill.intra.peff.net>
In-Reply-To: <CAETBDP42djjmSXeLig6mcRJVR0YMPnDUfCJT4z8SU==Ei62N4w@mail.gmail.com>

On Wed, Feb 27, 2019 at 10:46:49PM -0400, Brandon Richardson wrote:

Show 12 quoted lines

> > If we are going to go this route, I think you might actually want macros
> > that take both "unset" and "args" and make sure that we're not in a
> > situation the callback doesn't expect (e.g., "!unset && !arg"). That
> > lets us continue to declare those at the top of the callback.
> 
> In doing a quick search, I found a fair number instances of this:
> ...
> BUG_ON_OPT_NEG(unset);
> 
> if (!arg)
>      return -1;
> ...

Those are probably my fault. The originals guarded against an unexpected "unset" by checking "!arg" and returning an error. But it made the compiler's -Wunused-parameter complain, so I added the BUG_ON_OPT_NEG() calls as an assertion. At that point the "if (!arg)" could never trigger, and could have been removed.

> So a macro like this could be useful. I've also found a few instances of this:
> 
> BUG_ON_OPT_NEG(unset);
> BUG_ON_OPT_ARG(arg);

These ones are different. The second one is checking that "arg" _is_ NULL (i.e., we expect that the options struct provided the right flag to disallow an argument). And that's orthogonal to the unset flag, so it would not be right to conflate the two in a single macro.

-Peff