[PATCH] grep: use OPT_INTEGER_F for --max-depth

[PATCH] grep: use OPT_INTEGER_F for --max-depth

[René Scharfe]

a91f453f64 (grep: Add --max-depth option., 2009-07-22) added the option
--max-depth, defining it using a positional struct option initializer of
type OPTION_INTEGER.  It also sets defval to 1 for some reason, but that
value would only be used if the flag PARSE_OPT_OPTARG was given.

Use the macro OPT_INTEGER_F instead to standardize the definition and
specify only the necessary values.  This also normalizes argh to N_("n")
as a side-effect, which is OK.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/grep.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 50e712a184..f5f5f6dbe1 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -924,9 +924,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			 N_("process binary files with textconv filters")),
 		OPT_SET_INT('r', "recursive", &opt.max_depth,
 			    N_("search in subdirectories (default)"), -1),
-		{ OPTION_INTEGER, 0, "max-depth", &opt.max_depth, N_("depth"),
-			N_("descend at most <depth> levels"), PARSE_OPT_NONEG,
-			NULL, 1 },
+		OPT_INTEGER_F(0, "max-depth", &opt.max_depth,
+			N_("descend at most <n> levels"), PARSE_OPT_NONEG),
 		OPT_GROUP(""),
 		OPT_SET_INT('E', "extended-regexp", &opt.pattern_type_option,
 			    N_("use extended POSIX regular expressions"),
--
2.42.0

Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth

[Jeff King]

On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:

> a91f453f64 (grep: Add --max-depth option., 2009-07-22) added the option
> --max-depth, defining it using a positional struct option initializer of
> type OPTION_INTEGER.  It also sets defval to 1 for some reason, but that
> value would only be used if the flag PARSE_OPT_OPTARG was given.
> 
> Use the macro OPT_INTEGER_F instead to standardize the definition and
> specify only the necessary values.  This also normalizes argh to N_("n")
> as a side-effect, which is OK.

This looks correct to me (and an improvement in readability). In
general, I wonder how many of the results from:

  git grep '{ OPTION'

could be converted to use the macros and end up more readable. There are
a number of OPTARG ones, which I guess can't use macros. Looks like
there are a handful of others (mostly for OPT_HIDDEN).

-Peff

Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth

[René Scharfe]

Am 05.09.23 um 09:21 schrieb Jeff King:
> On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:
>
> In general, I wonder how many of the results from:
>
>   git grep '{ OPTION'
>
> could be converted to use the macros and end up more readable. There are
> a number of OPTARG ones, which I guess can't use macros. Looks like
> there are a handful of others (mostly for OPT_HIDDEN).

Indeed, and I have a semantic patch for that, but mostly because the
macros allow injecting a type check.

OPTARG would need a new macro to allow specifying the default value.  Or
is there a variadic macro trick that we could use?

René

Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth

[Jeff King]

On Thu, Sep 07, 2023 at 10:20:53PM +0200, René Scharfe wrote:

> Am 05.09.23 um 09:21 schrieb Jeff King:
> > On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:
> >
> > In general, I wonder how many of the results from:
> >
> >   git grep '{ OPTION'
> >
> > could be converted to use the macros and end up more readable. There are
> > a number of OPTARG ones, which I guess can't use macros. Looks like
> > there are a handful of others (mostly for OPT_HIDDEN).
> 
> Indeed, and I have a semantic patch for that, but mostly because the
> macros allow injecting a type check.
> 
> OPTARG would need a new macro to allow specifying the default value.  Or
> is there a variadic macro trick that we could use?

Hmm, I had just assumed OPTARG was a lost cause (or we would need an
"OPTARG" variant of each macro; yuck).

I suspect variadic macros could be made to work, but you'd lose some
compile-time safety. If I say:

  OPT_BOOL('x', NULL, &v, NULL, "turn on x")

now, the compiler will complain about the number of arguments. In a
variadic world, it silently ignores the final one. I feel like I've made
this kind of error before (e.g., when switching to/from _F variants, or
between types).

You'd want some semantic check between what's in flags (i.e., is the
OPTARG flag set), but I think that's beyond what the compiler itself can
do (you could probably write a coccinelle rule for it, though).

I think it also squats on the variadic concept for the macro, so that no
other features can use it. I.e., if you accidentally give _two_ extra
arguments, I don't know that we can parse them out individually.

So yeah, I think you'd really want a separate macro. The combinations
start to add up (or multiply up, if you prefer ;) ). They _could_ be
generated mechanically, I think, as they can all be implemented in terms
of a master macro that knows about all features:

   #define OPT_BOOL_F(s, l, v, h, f) OPT_BOOL_ALL(s, l, v, h, f, 0)
   #define OPT_BOOL(s, l, v, h, f) OPT_BOOL_F(s, l, v, h, 0)
   #define OPT_BOOL_OPTARG_F(s, l, v, h, arg) OPT_BOOL_ALL(s, l, v, h, f | PARSE_OPT_OPTARG, arg)
   #define OPT_BOOL_OPTARG(s, l, v, h, arg) OPT_BOOL_OPTARG_F(s, l, v, h, 0, arg)

But I'm not sure if cpp is up to that, or if I'd want to see what the
resulting code looks like.

-Peff

Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth

[René Scharfe]

Am 07.09.23 um 22:40 schrieb Jeff King:
> On Thu, Sep 07, 2023 at 10:20:53PM +0200, René Scharfe wrote:
>
>> Am 05.09.23 um 09:21 schrieb Jeff King:
>>> On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:
>>>
>>> In general, I wonder how many of the results from:
>>>
>>>   git grep '{ OPTION'
>>>
>>> could be converted to use the macros and end up more readable. There are
>>> a number of OPTARG ones, which I guess can't use macros. Looks like
>>> there are a handful of others (mostly for OPT_HIDDEN).
>>
>> Indeed, and I have a semantic patch for that, but mostly because the
>> macros allow injecting a type check.
>>
>> OPTARG would need a new macro to allow specifying the default value.  Or
>> is there a variadic macro trick that we could use?
>
> Hmm, I had just assumed OPTARG was a lost cause (or we would need an
> "OPTARG" variant of each macro; yuck).

Only for OPT_INTEGER and OPT_STRING AFAICS.

> I suspect variadic macros could be made to work, but you'd lose some
> compile-time safety. If I say:
>
>   OPT_BOOL('x', NULL, &v, NULL, "turn on x")
>
> now, the compiler will complain about the number of arguments. In a
> variadic world, it silently ignores the final one. I feel like I've made
> this kind of error before (e.g., when switching to/from _F variants, or
> between types).

OPT_BOOL has PARSE_OPT_NOARG.  Just saying.

It's true that a macro that accepts a variable number of arguments would
accept accidental extra arguments of the right type, but I don't see how
it would ignore excessive ones.

> You'd want some semantic check between what's in flags (i.e., is the
> OPTARG flag set), but I think that's beyond what the compiler itself can
> do (you could probably write a coccinelle rule for it, though).

Actually I'd want the macro to set that flag for me.

> I think it also squats on the variadic concept for the macro, so that no
> other features can use it. I.e., if you accidentally give _two_ extra
> arguments, I don't know that we can parse them out individually.

In case of an accident I'd just expect a compiler error.  A cryptic one,
probably, alas, but no silence.

I was thinking more about something like the solutions discussed at
https://stackoverflow.com/questions/47674663/variable-arguments-inside-a-macro.
It allows selecting variants based on argument count.

That could look like this (untested except on https://godbolt.org/; the
EVALs are needed for MSVC for some reason):

#define OPT_INTEGER_FULL(s, l, v, h, f, d) { \
	.type = OPTION_INTEGER, \
	.short_name = (s), \
	.long_name = (l), \
	.value = (v), \
	.argh = N_("n"), \
	.help = (h), \
	.flags = (f), \
	.defval = (d), \
}
#define OPT_INTEGER_4(s, l, v, h) \
	OPT_INTEGER_FULL(s, l, v, h, 0, 0)
#define OPT_INTEGER_5(s, l, v, h, f) \
	OPT_INTEGER_FULL(s, l, v, h, f, 0)
#define OPT_INTEGER_6(s, l, v, h, f, d) \
	OPT_INTEGER_FULL(s, l, v, h, (f) | PARSE_OPT_OPTARG, d)
#define EVAL(x) x
#define SEVENTH(_1, _2, _3, _4, _5, _6, x, ...) x
#define OPT_INTEGER(...) \
	EVAL(EVAL(SEVENTH(__VA_ARGS__, OPT_INTEGER_6, OPT_INTEGER_5, OPT_INTEGER_4, 0))(__VA_ARGS__))

So OPT_INTEGER(s, l, v, h) would be the same as before.  Add an argument
and it becomes current OPT_INTEGER_F, add another one and it acts as
your _OPTARG_F variant.

> So yeah, I think you'd really want a separate macro. The combinations
> start to add up (or multiply up, if you prefer ;) ). They _could_ be
> generated mechanically, I think, as they can all be implemented in terms
> of a master macro that knows about all features:
>
>    #define OPT_BOOL_F(s, l, v, h, f) OPT_BOOL_ALL(s, l, v, h, f, 0)
>    #define OPT_BOOL(s, l, v, h, f) OPT_BOOL_F(s, l, v, h, 0)

The "f" arg needs to go...

>    #define OPT_BOOL_OPTARG_F(s, l, v, h, arg) OPT_BOOL_ALL(s, l, v, h, f | PARSE_OPT_OPTARG, arg)

... here, possibly.

>    #define OPT_BOOL_OPTARG(s, l, v, h, arg) OPT_BOOL_OPTARG_F(s, l, v, h, 0, arg)
>
> But I'm not sure if cpp is up to that, or if I'd want to see what the
> resulting code looks like.

You mean having a macro define another macro?  I don't think that's
possible.

René

Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth

[René Scharfe]

Am 09.09.23 um 00:28 schrieb René Scharfe:
> Am 07.09.23 um 22:40 schrieb Jeff King:
>
> I was thinking more about something like the solutions discussed at
> https://stackoverflow.com/questions/47674663/variable-arguments-inside-a-macro.
> It allows selecting variants based on argument count.
>
> That could look like this (untested except on https://godbolt.org/; the
> EVALs are needed for MSVC for some reason):
>
> #define OPT_INTEGER_FULL(s, l, v, h, f, d) { \
> 	.type = OPTION_INTEGER, \
> 	.short_name = (s), \
> 	.long_name = (l), \
> 	.value = (v), \
> 	.argh = N_("n"), \
> 	.help = (h), \
> 	.flags = (f), \
> 	.defval = (d), \
> }
> #define OPT_INTEGER_4(s, l, v, h) \
> 	OPT_INTEGER_FULL(s, l, v, h, 0, 0)
> #define OPT_INTEGER_5(s, l, v, h, f) \
> 	OPT_INTEGER_FULL(s, l, v, h, f, 0)
> #define OPT_INTEGER_6(s, l, v, h, f, d) \
> 	OPT_INTEGER_FULL(s, l, v, h, (f) | PARSE_OPT_OPTARG, d)
> #define EVAL(x) x
> #define SEVENTH(_1, _2, _3, _4, _5, _6, x, ...) x
> #define OPT_INTEGER(...) \
> 	EVAL(EVAL(SEVENTH(__VA_ARGS__, OPT_INTEGER_6, OPT_INTEGER_5, OPT_INTEGER_4, 0))(__VA_ARGS__))
>
> So OPT_INTEGER(s, l, v, h) would be the same as before.  Add an argument
> and it becomes current OPT_INTEGER_F, add another one and it acts as
> your _OPTARG_F variant.

Should we actually do something like that?  Probably not.  At least it
doesn't help with my goals of simplicity and safety.  (I get sidetracked
so easily..)

>> So yeah, I think you'd really want a separate macro. The combinations
>> start to add up (or multiply up, if you prefer ;) ). They _could_ be
>> generated mechanically, I think, as they can all be implemented in terms
>> of a master macro that knows about all features:
>>
>>    #define OPT_BOOL_F(s, l, v, h, f) OPT_BOOL_ALL(s, l, v, h, f, 0)
>>    #define OPT_BOOL(s, l, v, h, f) OPT_BOOL_F(s, l, v, h, 0)
>
> The "f" arg needs to go...
>
>>    #define OPT_BOOL_OPTARG_F(s, l, v, h, arg) OPT_BOOL_ALL(s, l, v, h, f | PARSE_OPT_OPTARG, arg)
>
> ... here, possibly.
>
>>    #define OPT_BOOL_OPTARG(s, l, v, h, arg) OPT_BOOL_OPTARG_F(s, l, v, h, 0, arg)

Or we could use designated initializers directly.  It would improve
readability at the cost of some verbosity.  We could make it a bit
less verbose by by setting some flags implicitly based on type (e.g.
set PARSE_OPT_OPTARG if defval is set for an OPTION_INTEGER option).

René

Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth

[Jeff King]

On Sat, Sep 09, 2023 at 12:28:20AM +0200, René Scharfe wrote:

> >> OPTARG would need a new macro to allow specifying the default value.  Or
> >> is there a variadic macro trick that we could use?
> >
> > Hmm, I had just assumed OPTARG was a lost cause (or we would need an
> > "OPTARG" variant of each macro; yuck).
> 
> Only for OPT_INTEGER and OPT_STRING AFAICS.

True, my use of BOOL was obviously dumb, as it wouldn't have arguments.
But in theory anything that takes an argument could have an OPTARG
variant. So that would include special stuff like OPT_EXPIRY_DATE,
OPT_FILENAME, and so on. Though I would not be surprised if we currently
only use it for string/integer.

> It's true that a macro that accepts a variable number of arguments would
> accept accidental extra arguments of the right type, but I don't see how
> it would ignore excessive ones.

The macro itself wouldn't notice, but I guess the generated code would
probably complain about getting "(foo,bar)" as the initializer, if you
really sent to many.

But I was more worried about an error where you accidentally give an
extra argument. Right now that's an error, but would it be quietly
shifted into the OPTARG slot?

> > You'd want some semantic check between what's in flags (i.e., is the
> > OPTARG flag set), but I think that's beyond what the compiler itself can
> > do (you could probably write a coccinelle rule for it, though).
> 
> Actually I'd want the macro to set that flag for me.

For a dedicated OPT_STRING_OPTARG(), I'd agree. For OPT_STRING() that
uses varargs, I'm more on the fence (because of the cross-checking
above; now we are getting into "accidentally adding a parameter is
quietly accepted" territory).

I dunno. Maybe saving some keystrokes is worth it, but having to say
both OPTARG _and_ provide the extra argument makes things less subtle.

> I was thinking more about something like the solutions discussed at
> https://stackoverflow.com/questions/47674663/variable-arguments-inside-a-macro.
> It allows selecting variants based on argument count.
> [...]
> So OPT_INTEGER(s, l, v, h) would be the same as before.  Add an argument
> and it becomes current OPT_INTEGER_F, add another one and it acts as
> your _OPTARG_F variant.

Ah, yeah, I've seen something like this before. I do think it would
work as you're suggesting. I'm just not sure if being verbose and
explicit is better than trying to be clever here.

-Peff