{"thread":{"id":"60187","subject":"[PATCH] grep: use OPT_INTEGER_F for --max-depth","startedAt":"2023-09-02T18:55:01Z","lastAt":"2023-09-12T07:52:15Z","messageCount":7,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"481332","messageId":"4d2eb736-4f34-18f8-2eb7-20e7f7b8c2f8@web.de","threadId":"60187","inReplyTo":null,"subject":"[PATCH] grep: use OPT_INTEGER_F for --max-depth","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-02T18:54:54Z","receivedAt":"2023-09-02T18:55:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"a91f453f64 (grep: Add --max-depth option., 2009-07-22) added the option\n--max-depth, defining it using a positional struct option initializer of\ntype OPTION_INTEGER.  It also sets defval to 1 for some reason, but that\nvalue would only be used if the flag PARSE_OPT_OPTARG was given.\n\nUse the macro OPT_INTEGER_F instead to standardize the definition and\nspecify only the necessary values.  This also normalizes argh to N_(\"n\")\nas a side-effect, which is OK.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/grep.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 50e712a184..f5f5f6dbe1 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -924,9 +924,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"process binary files with textconv filters\")),\n \t\tOPT_SET_INT('r', \"recursive\", &opt.max_depth,\n \t\t\t    N_(\"search in subdirectories (default)\"), -1),\n-\t\t{ OPTION_INTEGER, 0, \"max-depth\", &opt.max_depth, N_(\"depth\"),\n-\t\t\tN_(\"descend at most <depth> levels\"), PARSE_OPT_NONEG,\n-\t\t\tNULL, 1 },\n+\t\tOPT_INTEGER_F(0, \"max-depth\", &opt.max_depth,\n+\t\t\tN_(\"descend at most <n> levels\"), PARSE_OPT_NONEG),\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_SET_INT('E', \"extended-regexp\", &opt.pattern_type_option,\n \t\t\t    N_(\"use extended POSIX regular expressions\"),\n--\n2.42.0\n"},{"id":"481392","messageId":"20230905072122.GG199565@coredump.intra.peff.net","threadId":"60187","inReplyTo":"4d2eb736-4f34-18f8-2eb7-20e7f7b8c2f8@web.de","subject":"Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-05T07:21:22Z","receivedAt":"2023-09-05T16:00:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:\n\n> a91f453f64 (grep: Add --max-depth option., 2009-07-22) added the option\n> --max-depth, defining it using a positional struct option initializer of\n> type OPTION_INTEGER.  It also sets defval to 1 for some reason, but that\n> value would only be used if the flag PARSE_OPT_OPTARG was given.\n> \n> Use the macro OPT_INTEGER_F instead to standardize the definition and\n> specify only the necessary values.  This also normalizes argh to N_(\"n\")\n> as a side-effect, which is OK.\n\nThis looks correct to me (and an improvement in readability). In\ngeneral, I wonder how many of the results from:\n\n  git grep '{ OPTION'\n\ncould be converted to use the macros and end up more readable. There are\na number of OPTARG ones, which I guess can't use macros. Looks like\nthere are a handful of others (mostly for OPT_HIDDEN).\n\n-Peff\n"},{"id":"481513","messageId":"724641d2-1cba-3768-6008-01e8a1cdca4e@web.de","threadId":"60187","inReplyTo":"20230905072122.GG199565@coredump.intra.peff.net","subject":"Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-07T20:20:53Z","receivedAt":"2023-09-07T20:21:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.09.23 um 09:21 schrieb Jeff King:\n> On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:\n>\n> In general, I wonder how many of the results from:\n>\n>   git grep '{ OPTION'\n>\n> could be converted to use the macros and end up more readable. There are\n> a number of OPTARG ones, which I guess can't use macros. Looks like\n> there are a handful of others (mostly for OPT_HIDDEN).\n\nIndeed, and I have a semantic patch for that, but mostly because the\nmacros allow injecting a type check.\n\nOPTARG would need a new macro to allow specifying the default value.  Or\nis there a variadic macro trick that we could use?\n\nRené\n"},{"id":"481520","messageId":"20230907204027.GA941945@coredump.intra.peff.net","threadId":"60187","inReplyTo":"724641d2-1cba-3768-6008-01e8a1cdca4e@web.de","subject":"Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-07T20:40:27Z","receivedAt":"2023-09-07T20:40:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 07, 2023 at 10:20:53PM +0200, René Scharfe wrote:\n\n> Am 05.09.23 um 09:21 schrieb Jeff King:\n> > On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:\n> >\n> > In general, I wonder how many of the results from:\n> >\n> >   git grep '{ OPTION'\n> >\n> > could be converted to use the macros and end up more readable. There are\n> > a number of OPTARG ones, which I guess can't use macros. Looks like\n> > there are a handful of others (mostly for OPT_HIDDEN).\n> \n> Indeed, and I have a semantic patch for that, but mostly because the\n> macros allow injecting a type check.\n> \n> OPTARG would need a new macro to allow specifying the default value.  Or\n> is there a variadic macro trick that we could use?\n\nHmm, I had just assumed OPTARG was a lost cause (or we would need an\n\"OPTARG\" variant of each macro; yuck).\n\nI suspect variadic macros could be made to work, but you'd lose some\ncompile-time safety. If I say:\n\n  OPT_BOOL('x', NULL, &v, NULL, \"turn on x\")\n\nnow, the compiler will complain about the number of arguments. In a\nvariadic world, it silently ignores the final one. I feel like I've made\nthis kind of error before (e.g., when switching to/from _F variants, or\nbetween types).\n\nYou'd want some semantic check between what's in flags (i.e., is the\nOPTARG flag set), but I think that's beyond what the compiler itself can\ndo (you could probably write a coccinelle rule for it, though).\n\nI think it also squats on the variadic concept for the macro, so that no\nother features can use it. I.e., if you accidentally give _two_ extra\narguments, I don't know that we can parse them out individually.\n\nSo yeah, I think you'd really want a separate macro. The combinations\nstart to add up (or multiply up, if you prefer ;) ). They _could_ be\ngenerated mechanically, I think, as they can all be implemented in terms\nof a master macro that knows about all features:\n\n   #define OPT_BOOL_F(s, l, v, h, f) OPT_BOOL_ALL(s, l, v, h, f, 0)\n   #define OPT_BOOL(s, l, v, h, f) OPT_BOOL_F(s, l, v, h, 0)\n   #define OPT_BOOL_OPTARG_F(s, l, v, h, arg) OPT_BOOL_ALL(s, l, v, h, f | PARSE_OPT_OPTARG, arg)\n   #define OPT_BOOL_OPTARG(s, l, v, h, arg) OPT_BOOL_OPTARG_F(s, l, v, h, 0, arg)\n\nBut I'm not sure if cpp is up to that, or if I'd want to see what the\nresulting code looks like.\n\n-Peff\n"},{"id":"481580","messageId":"cec47733-5b15-6ca7-adaf-7f3216ad178b@web.de","threadId":"60187","inReplyTo":"20230907204027.GA941945@coredump.intra.peff.net","subject":"Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-08T22:28:20Z","receivedAt":"2023-09-08T22:28:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.09.23 um 22:40 schrieb Jeff King:\n> On Thu, Sep 07, 2023 at 10:20:53PM +0200, René Scharfe wrote:\n>\n>> Am 05.09.23 um 09:21 schrieb Jeff King:\n>>> On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:\n>>>\n>>> In general, I wonder how many of the results from:\n>>>\n>>>   git grep '{ OPTION'\n>>>\n>>> could be converted to use the macros and end up more readable. There are\n>>> a number of OPTARG ones, which I guess can't use macros. Looks like\n>>> there are a handful of others (mostly for OPT_HIDDEN).\n>>\n>> Indeed, and I have a semantic patch for that, but mostly because the\n>> macros allow injecting a type check.\n>>\n>> OPTARG would need a new macro to allow specifying the default value.  Or\n>> is there a variadic macro trick that we could use?\n>\n> Hmm, I had just assumed OPTARG was a lost cause (or we would need an\n> \"OPTARG\" variant of each macro; yuck).\n\nOnly for OPT_INTEGER and OPT_STRING AFAICS.\n\n> I suspect variadic macros could be made to work, but you'd lose some\n> compile-time safety. If I say:\n>\n>   OPT_BOOL('x', NULL, &v, NULL, \"turn on x\")\n>\n> now, the compiler will complain about the number of arguments. In a\n> variadic world, it silently ignores the final one. I feel like I've made\n> this kind of error before (e.g., when switching to/from _F variants, or\n> between types).\n\nOPT_BOOL has PARSE_OPT_NOARG.  Just saying.\n\nIt's true that a macro that accepts a variable number of arguments would\naccept accidental extra arguments of the right type, but I don't see how\nit would ignore excessive ones.\n\n> You'd want some semantic check between what's in flags (i.e., is the\n> OPTARG flag set), but I think that's beyond what the compiler itself can\n> do (you could probably write a coccinelle rule for it, though).\n\nActually I'd want the macro to set that flag for me.\n\n> I think it also squats on the variadic concept for the macro, so that no\n> other features can use it. I.e., if you accidentally give _two_ extra\n> arguments, I don't know that we can parse them out individually.\n\nIn case of an accident I'd just expect a compiler error.  A cryptic one,\nprobably, alas, but no silence.\n\nI was thinking more about something like the solutions discussed at\nhttps://stackoverflow.com/questions/47674663/variable-arguments-inside-a-macro.\nIt allows selecting variants based on argument count.\n\nThat could look like this (untested except on https://godbolt.org/; the\nEVALs are needed for MSVC for some reason):\n\n#define OPT_INTEGER_FULL(s, l, v, h, f, d) { \\\n\t.type = OPTION_INTEGER, \\\n\t.short_name = (s), \\\n\t.long_name = (l), \\\n\t.value = (v), \\\n\t.argh = N_(\"n\"), \\\n\t.help = (h), \\\n\t.flags = (f), \\\n\t.defval = (d), \\\n}\n#define OPT_INTEGER_4(s, l, v, h) \\\n\tOPT_INTEGER_FULL(s, l, v, h, 0, 0)\n#define OPT_INTEGER_5(s, l, v, h, f) \\\n\tOPT_INTEGER_FULL(s, l, v, h, f, 0)\n#define OPT_INTEGER_6(s, l, v, h, f, d) \\\n\tOPT_INTEGER_FULL(s, l, v, h, (f) | PARSE_OPT_OPTARG, d)\n#define EVAL(x) x\n#define SEVENTH(_1, _2, _3, _4, _5, _6, x, ...) x\n#define OPT_INTEGER(...) \\\n\tEVAL(EVAL(SEVENTH(__VA_ARGS__, OPT_INTEGER_6, OPT_INTEGER_5, OPT_INTEGER_4, 0))(__VA_ARGS__))\n\nSo OPT_INTEGER(s, l, v, h) would be the same as before.  Add an argument\nand it becomes current OPT_INTEGER_F, add another one and it acts as\nyour _OPTARG_F variant.\n\n> So yeah, I think you'd really want a separate macro. The combinations\n> start to add up (or multiply up, if you prefer ;) ). They _could_ be\n> generated mechanically, I think, as they can all be implemented in terms\n> of a master macro that knows about all features:\n>\n>    #define OPT_BOOL_F(s, l, v, h, f) OPT_BOOL_ALL(s, l, v, h, f, 0)\n>    #define OPT_BOOL(s, l, v, h, f) OPT_BOOL_F(s, l, v, h, 0)\n\nThe \"f\" arg needs to go...\n\n>    #define OPT_BOOL_OPTARG_F(s, l, v, h, arg) OPT_BOOL_ALL(s, l, v, h, f | PARSE_OPT_OPTARG, arg)\n\n... here, possibly.\n\n>    #define OPT_BOOL_OPTARG(s, l, v, h, arg) OPT_BOOL_OPTARG_F(s, l, v, h, 0, arg)\n>\n> But I'm not sure if cpp is up to that, or if I'd want to see what the\n> resulting code looks like.\n\nYou mean having a macro define another macro?  I don't think that's\npossible.\n\nRené\n"},{"id":"481633","messageId":"e09ce805-056c-c3fb-6955-69b6ae0a2cb1@web.de","threadId":"60187","inReplyTo":"cec47733-5b15-6ca7-adaf-7f3216ad178b@web.de","subject":"Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-09T21:14:40Z","receivedAt":"2023-09-09T21:14:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.09.23 um 00:28 schrieb René Scharfe:\n> Am 07.09.23 um 22:40 schrieb Jeff King:\n>\n> I was thinking more about something like the solutions discussed at\n> https://stackoverflow.com/questions/47674663/variable-arguments-inside-a-macro.\n> It allows selecting variants based on argument count.\n>\n> That could look like this (untested except on https://godbolt.org/; the\n> EVALs are needed for MSVC for some reason):\n>\n> #define OPT_INTEGER_FULL(s, l, v, h, f, d) { \\\n> \t.type = OPTION_INTEGER, \\\n> \t.short_name = (s), \\\n> \t.long_name = (l), \\\n> \t.value = (v), \\\n> \t.argh = N_(\"n\"), \\\n> \t.help = (h), \\\n> \t.flags = (f), \\\n> \t.defval = (d), \\\n> }\n> #define OPT_INTEGER_4(s, l, v, h) \\\n> \tOPT_INTEGER_FULL(s, l, v, h, 0, 0)\n> #define OPT_INTEGER_5(s, l, v, h, f) \\\n> \tOPT_INTEGER_FULL(s, l, v, h, f, 0)\n> #define OPT_INTEGER_6(s, l, v, h, f, d) \\\n> \tOPT_INTEGER_FULL(s, l, v, h, (f) | PARSE_OPT_OPTARG, d)\n> #define EVAL(x) x\n> #define SEVENTH(_1, _2, _3, _4, _5, _6, x, ...) x\n> #define OPT_INTEGER(...) \\\n> \tEVAL(EVAL(SEVENTH(__VA_ARGS__, OPT_INTEGER_6, OPT_INTEGER_5, OPT_INTEGER_4, 0))(__VA_ARGS__))\n>\n> So OPT_INTEGER(s, l, v, h) would be the same as before.  Add an argument\n> and it becomes current OPT_INTEGER_F, add another one and it acts as\n> your _OPTARG_F variant.\n\nShould we actually do something like that?  Probably not.  At least it\ndoesn't help with my goals of simplicity and safety.  (I get sidetracked\nso easily..)\n\n>> So yeah, I think you'd really want a separate macro. The combinations\n>> start to add up (or multiply up, if you prefer ;) ). They _could_ be\n>> generated mechanically, I think, as they can all be implemented in terms\n>> of a master macro that knows about all features:\n>>\n>>    #define OPT_BOOL_F(s, l, v, h, f) OPT_BOOL_ALL(s, l, v, h, f, 0)\n>>    #define OPT_BOOL(s, l, v, h, f) OPT_BOOL_F(s, l, v, h, 0)\n>\n> The \"f\" arg needs to go...\n>\n>>    #define OPT_BOOL_OPTARG_F(s, l, v, h, arg) OPT_BOOL_ALL(s, l, v, h, f | PARSE_OPT_OPTARG, arg)\n>\n> ... here, possibly.\n>\n>>    #define OPT_BOOL_OPTARG(s, l, v, h, arg) OPT_BOOL_OPTARG_F(s, l, v, h, 0, arg)\n\nOr we could use designated initializers directly.  It would improve\nreadability at the cost of some verbosity.  We could make it a bit\nless verbose by by setting some flags implicitly based on type (e.g.\nset PARSE_OPT_OPTARG if defval is set for an OPTION_INTEGER option).\n\nRené\n"},{"id":"481745","messageId":"20230912075145.GA1630538@coredump.intra.peff.net","threadId":"60187","inReplyTo":"cec47733-5b15-6ca7-adaf-7f3216ad178b@web.de","subject":"Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-12T07:51:45Z","receivedAt":"2023-09-12T07:52:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 09, 2023 at 12:28:20AM +0200, René Scharfe wrote:\n\n> >> OPTARG would need a new macro to allow specifying the default value.  Or\n> >> is there a variadic macro trick that we could use?\n> >\n> > Hmm, I had just assumed OPTARG was a lost cause (or we would need an\n> > \"OPTARG\" variant of each macro; yuck).\n> \n> Only for OPT_INTEGER and OPT_STRING AFAICS.\n\nTrue, my use of BOOL was obviously dumb, as it wouldn't have arguments.\nBut in theory anything that takes an argument could have an OPTARG\nvariant. So that would include special stuff like OPT_EXPIRY_DATE,\nOPT_FILENAME, and so on. Though I would not be surprised if we currently\nonly use it for string/integer.\n\n> It's true that a macro that accepts a variable number of arguments would\n> accept accidental extra arguments of the right type, but I don't see how\n> it would ignore excessive ones.\n\nThe macro itself wouldn't notice, but I guess the generated code would\nprobably complain about getting \"(foo,bar)\" as the initializer, if you\nreally sent to many.\n\nBut I was more worried about an error where you accidentally give an\nextra argument. Right now that's an error, but would it be quietly\nshifted into the OPTARG slot?\n\n> > You'd want some semantic check between what's in flags (i.e., is the\n> > OPTARG flag set), but I think that's beyond what the compiler itself can\n> > do (you could probably write a coccinelle rule for it, though).\n> \n> Actually I'd want the macro to set that flag for me.\n\nFor a dedicated OPT_STRING_OPTARG(), I'd agree. For OPT_STRING() that\nuses varargs, I'm more on the fence (because of the cross-checking\nabove; now we are getting into \"accidentally adding a parameter is\nquietly accepted\" territory).\n\nI dunno. Maybe saving some keystrokes is worth it, but having to say\nboth OPTARG _and_ provide the extra argument makes things less subtle.\n\n> I was thinking more about something like the solutions discussed at\n> https://stackoverflow.com/questions/47674663/variable-arguments-inside-a-macro.\n> It allows selecting variants based on argument count.\n> [...]\n> So OPT_INTEGER(s, l, v, h) would be the same as before.  Add an argument\n> and it becomes current OPT_INTEGER_F, add another one and it acts as\n> your _OPTARG_F variant.\n\nAh, yeah, I've seen something like this before. I do think it would\nwork as you're suggesting. I'm just not sure if being verbose and\nexplicit is better than trying to be clever here.\n\n-Peff\n"}]}