| Message ID | 4d2eb736-4f34-18f8-2eb7-20e7f7b8c2f8@web.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2a63c79dae72b25420bc71116bef7436fd846758 |
| Headers | show |
| Series | grep: use OPT_INTEGER_F for --max-depth | expand |
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
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é
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
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é
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é
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
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"),
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(-) -- 2.42.0