From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth
Date: Tue, 12 Sep 2023 03:51:45 -0400 [thread overview]
Message-ID: <20230912075145.GA1630538@coredump.intra.peff.net> (raw)
In-Reply-To: <cec47733-5b15-6ca7-adaf-7f3216ad178b@web.de>
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
prev parent reply other threads:[~2023-09-12 7:52 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-02 18:54 [PATCH] grep: use OPT_INTEGER_F for --max-depth René Scharfe
2023-09-05 7:21 ` Jeff King
2023-09-07 20:20 ` René Scharfe
2023-09-07 20:40 ` Jeff King
2023-09-08 22:28 ` René Scharfe
2023-09-09 21:14 ` René Scharfe
2023-09-12 7:51 ` Jeff King [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230912075145.GA1630538@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=l.s.r@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).