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

From: Jeff King <peff@peff.net>
Date: Mar 1, 2019, 19:09 UTC
Message-ID: <20190301190954.GG30847@sigill.intra.peff.net>
In-Reply-To: <20190301171304.2267-1-brandon1024.br@gmail.com>

On Fri, Mar 01, 2019 at 01:13:04PM -0400, Brandon Richardson wrote:

Show 5 quoted lines

> +/*
> + * Use this assertion for callbacks that expect to be called with NONEG,
> + * and require an argument be supplied.
> + */
> +#define BUG_ON_OPT_NEG_NOARG(unset, arg) do { \

I think this general concept is fine. It's a variant of BUG_ON_OPT_NEG(), so you'd use one or the other.

However, the implementation:

> +	if((!unset) && (!arg)) \
> +		BUG("option callback does not expect negation and requires an argument"); \

does not really make sense. If "!unset" is true, then we know that "!arg" will always be true as well. So this collapse down to "!unset", which is the same as BUG_ON_OPT_NEG().

I think you want an "OR". Or even separate conditions, since really this is just implying OPT_NEG(). In fact, you could implement and explain it like this:

diff --git a/parse-options.h b/parse-options.h
index 14fe32428e..d46f89305c 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -202,6 +202,18 @@ const char *optname(const struct option *opt, int flags);
 		BUG("option callback does not expect an argument"); \
 } while (0)
 
+/*
+ * 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)
+
 /*----- incremental advanced APIs -----*/
 
 enum {

-Peff