Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
- From
Jeff King <peff@peff.net>
- Date
- Dec 20, 2013, 07:04 UTC
- Message-ID
- <20131220070449.GA29717@sigill.intra.peff.net>
- In-Reply-To
- <52B3E8D4.1030805@viscovery.net>
On Fri, Dec 20, 2013 at 07:51:00AM +0100, Johannes Sixt wrote:
Show 15 quoted lines
> > for (i = 1; i < argc && *argv[i] == '-'; i++) {
> > const char *arg = argv[i];
> > + const char *optarg;
> >
> > - if (starts_with(arg, "--upload-pack=")) {
> > - args.uploadpack = arg + 14;
> > + if ((optarg = skip_prefix(arg, "--upload-pack=")) != NULL) {
> > + args.uploadpack = optarg;
>
> Quite frankly, I do not think this is an improvement. The old code is
> *MUCH* easier to understand because "starts_with" is clearly a predicate
> that is either true or false, but the code with "skip_prefix" is much
> heavier on the eye with its extra level of parenthesis. That it removes a
> hard-coded constant does not count much IMHO because it is very clear
> where the value comes from.Yeah, I agree that is unfortunate. Maybe we could have the best of both worlds, like:
if (starts_with(arg, "--upload-pack=", &optarg))
... use optarg ...Probably we do not want to call it just "starts_with", as quite a few callers to do not care about what comes next, and would just pass NULL.
I cannot seem to think of a good name, though, as the "with" means that obvious things like "starts_with_value" naturally parse as a single (nonsensical) sentence. Something like "parse_prefix" would work, but it is not as clearly a predicate as "starts_with" (but we have at least gotten rid of the extra parentheses).
Elsewhere in the thread, the concept was discussed of returning the full string to mean "did not match", which makes some other idioms simpler (but IMHO makes the simple cases like this even harder to read). My proposal splits the "start of string" out parameter from the boolean return, so it handles both cases naturally:
/* here we care if we saw the prefix, as above */
if (parse_prefix(foo, prefix, &the_rest))
.../* * and here we do not care, and just want to optionally strip the * prefix, and take the full value otherwise; we just have to ignore * the return value in this case. */ parse_prefix(foo, prefix, &foo);
-Peff