Re: [PATCH] Add a simple option parser.

[Alex Riesen <raa.lkml@gmail.com>]

Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> +static int opterror(struct option *opt, const char *reason, int flags)

"const struct option *opt"? You never modify the struct option itself,
only the values under the pointers it contains. Using const here will
allow the compiler to reuse string constants (not that there will be
much of the opportunity, but anyway) in the option arrays.

> +static int get_value(struct optparse_t *p, struct option *opt, int flags)

"const struct option *opt"?

> +static int parse_short_opt(struct optparse_t *p, struct option *options, int count)

"const struct option *options"?

> +int parse_options(int argc, const char **argv,
> +                  struct option *options, int count,
> +				  const char * const usagestr[], int flags)

"const struct option *options"?

> +void make_usage(const char * const usagestr[], struct option *opts, int cnt)

"const struct option *opts"?

Why not "const char *const *usagestr"? Especially if you change
"usagestr" (the pointer itself) later. "[]" is sometimes a hint that
the pointer itself should not be changed, being an array.

And you want make opts const.

BTW, it does not "make" usage. It calls the usage() or prints a usage
description. "make" implies it creates the "usage", which according to
the prototype is later nowhere to be found.

> +{
> +	struct strbuf sb;
> +
> +	strbuf_init(&sb, 4096);
> +	do {
> +		strbuf_addstr(&sb, *usagestr++);
> +		strbuf_addch(&sb, '\n');
> +	} while (*usagestr);

This will crash for empty usagestr, like  "{ NULL }". Was it
deliberately? (I'd make it deliberately, if I were you. I'd even used
cnt of opts, to force people to document all options).

> +     strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> +		    opts->help);
...
> +	usage(sb.buf);

BTW, if you just printed the usage message out (it is about usage of a
program, isn't it?) and called exit() everyone would be just as happy.
And you wouldn't have to include strbuf (it is the only use of it),
less code, too. It'd make simplier to stea^Wcopy your implementation,
which I like :)