Re: [PATCH] Add a simple option parser.

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 4865 bytes --]

On Sat, Oct 13, 2007 at 02:39:10PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 13 Oct 2007, Pierre Habouzit wrote:
> 
> > Aggregation of single switches is allowed:
> >   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
> 
> I'd be more interested in "-rC 0" working...  Is that supported, too?

  yes it is.

> > +static inline const char *get_arg(struct optparse_t *p)
> > +{
> > +	if (p->opt) {
> > +		const char *res = p->opt;
> > +		p->opt = NULL;
> > +		return res;
> > +	}
> > +	p->argc--;
> > +	return *++p->argv;
> > +}
> 
> This is only used once; I wonder if it is really that more readable having 
> this as a function in its own right.

  it's used twice, and it makes the code more readable I believe.

> > +static inline const char *skippfx(const char *str, const char *prefix)
> 
> Personally, I do not like abbreviations like that.  They do not save that 
> much screen estate (skip_prefix is only 4 characters longer, but much more 
> readable).  Same goes for "cnt" later.

  Ack I'll fix that.

> > +static int parse_long_opt(struct optparse_t *p, const char *arg,
> > +                          struct option *options, int count)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		const char *rest;
> > +		int flags = 0;
> > +		
> > +		if (!options[i].long_name)
> > +			continue;
> > +
> > +		rest = skippfx(arg, options[i].long_name);
> > +		if (!rest && !strncmp(arg, "no-", 3)) {
> > +			rest = skippfx(arg + 3, options[i].long_name);
> > +			flags |= OPT_SHORT;
> > +		}
> 
> Would this not be more intuitive as
> 
> 		if (!prefixcmp(arg, "no-")) {
> 			arg += 3;
> 			flags |= OPT_UNSET;
> 		}
> 		rest = skip_prefix(arg, options[i].long_name);

  Yes, that can be done indeed, but the point is, we have sometimes
option whose long-name is "no-foo" (because it's what makes sense) but I
can rework that.

> Hm?  (Note that I say UNSET, not SHORT... ;-)

  fsck, good catch.

> > +		if (!rest)
> > +			continue;
> > +		if (*rest) {
> > +			if (*rest != '=')
> > +				continue;
> 
> Is this really no error?  For example, "git log 
> --decorate-walls-and-roofs" would not fail...

  it would be an error, it will yield a "option not found".

> > +int parse_options(int argc, const char **argv,
> > +                  struct option *options, int count,
> > +				  const char * const usagestr[], int flags)
> 
> Please indent by the same amount.

  oops, stupid space vs. tab thing.

> > +		if (arg[1] != '-') {
> > +			optp.opt = arg + 1;
> > +			do {
> > +				if (*optp.opt == 'h')
> > +					make_usage(usagestr, options, count);
> 
> How about calling this "usage_with_options()"?  With that name I expected 
> make_usage() to return a strbuf.

  will do.

> > +		if (!arg[2]) { /* "--" */
> > +			if (!(flags & OPT_COPY_DASHDASH))
> > +				optp.argc--, optp.argv++;
> 
> I would prefer this as 
> 
> 			if (!(flags & OPT_COPY_DASHDASH)) {
> 				optp.argc--;
> 				optp.argv++;
> 			}
> 
> While I'm at it: could you use "args" instead of "optp"?  It is misleading 
> both in that it not only contains options (but other arguments, too) as in 
> that it is not a pointer (the trailing "p" is used as an indicator of that 
> very often, including git's source code).

  okay.

> In the same vein, OPT_COPY_DASHDASH should be named 
> PARSE_OPT_KEEP_DASHDASH.

  okay.

> 
> > +		if (opts->short_name) {
> > +			strbuf_addf(&sb, "-%c", opts->short_name);
> > +		}
> > +		if (opts->long_name) {
> > +			strbuf_addf(&sb, opts->short_name ? ", --%s" : "--%s",
> > +						opts->long_name);
> > +		}
> 
> Please lose the curly brackets.
> 
> > +		if (sb.len - pos <= USAGE_OPTS_WIDTH) {
> > +			int pad = USAGE_OPTS_WIDTH - (sb.len - pos) + USAGE_GAP;
> > +			strbuf_addf(&sb, "%*s%s\n", pad, "", opts->help);
> > +		} else {
> > +			strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> > +						opts->help);
> > +		}
> 
> Same here.  (And I'd also make sure that the lines are not that long.)

  okay.

> 
> > diff --git a/parse-options.h b/parse-options.h
> > new file mode 100644
> > index 0000000..4b33d17
> > --- /dev/null
> > +++ b/parse-options.h
> > @@ -0,0 +1,37 @@
> > +#ifndef PARSE_OPTIONS_H
> > +#define PARSE_OPTIONS_H
> > +
> > +enum option_type {
> > +	OPTION_BOOLEAN,
> 
> I know that I proposed "BOOLEAN", but actually, you use it more like an 
> "INCREMENTAL", right?

  yes, I don't like _BOOLEAN either, I would have prefered _FLAG or sth
like that. INCREMENTAL is just too long.

> Other than that: I like it very much.

:P

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]