On Fri, Dec 20, 2013 at 07:51:00AM +0100, Johannes Sixt wrote:
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html