patchgrep: use OPT_INTEGER_F for --max-depth

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

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

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

Am 05.09.23 um 09:21 schrieb Jeff King:

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é

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

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

Am 07.09.23 um 22:40 schrieb Jeff King:

Only for OPT_INTEGER and OPT_STRING AFAICS.

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.

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

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.

The "f" arg needs to go...

... here, possibly.

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

René

Am 09.09.23 um 00:28 schrieb René Scharfe:

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

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é

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

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.

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?

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.

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