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

From: Brandon Richardson <brandon1024.br@gmail.com>
Date: Feb 28, 2019, 02:46 UTC
Message-ID: <CAETBDP42djjmSXeLig6mcRJVR0YMPnDUfCJT4z8SU==Ei62N4w@mail.gmail.com>
In-Reply-To: <20190227163522.GA25188@sigill.intra.peff.net>

Hi Jeff,

Show 12 quoted lines

> One of the reasons I did not bother with that condition when I added the
> OPT_NEG() and OPT_ARG() variants is that you can only get an unexpected
> NULL argument if you explicitly give the NOARG or OPTARG flags. So it's
> very easy to _forget_ to give such a flag, because you simply aren't
> thinking about that case, and your callback is buggy by default.
>
> But it's rare to actually think to give one of those flags, but then
> forget to handle it in your callback.
>
> So I'm not entirely opposed, but it does feel weird to add such a macro
> without then using it in the 99% of callbacks which expect arg to be
> non-NULL.

I'd like to agree with you here, especially given that commit-tree is a rather small part of project source. Experimenting with it a bit, I found using BUG_ON_OPT_NOARG() to be a big clunky. Like you said, we could end up with some less-than-ideal usage. If I were to use this in commit-tree, it would look something like this, which isn't very appealing:

static int callback(const struct option *opt, const char *arg, int unset)
{
     ...
     BUG_ON_OPT_NEG(unset);
     BUG_ON_OPT_NO_ARG(arg);
     ...

However, I do still see a use case for a new macro for options that cannot be unset and arguments that must not be NULL.

> 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;
...

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);

Perhaps two new macros BUG_ON_OPT_NEG_NO_ARG() ("!unset || !arg") and BUG_ON_OPT_NEG_ARG() ("!unset || arg")? I'm not a big fan of those names though.

Brandon