Re: [PATCH] Add a simple option parser.
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?
Show 29 quoted lines
> diff --git a/parse-options.c b/parse-options.c
> new file mode 100644
> index 0000000..07abb50
> --- /dev/null
> +++ b/parse-options.c
> @@ -0,0 +1,227 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +#include "strbuf.h"
> +
> +#define OPT_SHORT 1
> +#define OPT_UNSET 2
> +
> +struct optparse_t {
> + const char **argv;
> + int argc;
> + const char *opt;
> +};
> +
> +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.
> +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.
Show 41 quoted lines
> +static int get_value(struct optparse_t *p, struct option *opt, int flags)
> +{
> + if (p->opt && (flags & OPT_UNSET))
> + return opterror(opt, "takes no value", flags);
> +
> + switch (opt->type) {
> + case OPTION_BOOLEAN:
> + if (!(flags & OPT_SHORT) && p->opt)
> + return opterror(opt, "takes no value", flags);
> + if (flags & OPT_UNSET) {
> + *(int *)opt->value = 0;
> + } else {
> + (*(int *)opt->value)++;
> + }
> + return 0;
> +
> + case OPTION_STRING:
> + if (flags & OPT_UNSET) {
> + *(const char **)opt->value = (const char *)NULL;
> + } else {
> + if (!p->opt && p->argc < 1)
> + return opterror(opt, "requires a value", flags);
> + *(const char **)opt->value = get_arg(p);
> + }
> + return 0;
> +
> + case OPTION_INTEGER:
> + if (flags & OPT_UNSET) {
> + *(int *)opt->value = 0;
> + } else {
> + const char *s;
> + if (!p->opt && p->argc < 1)
> + return opterror(opt, "requires a value", flags);
> + *(int *)opt->value = strtol(*p->argv, (char **)&s, 10);
> + if (*s)
> + return opterror(opt, "expects a numerical value", flags);
> + }
> + return 0;
> +
> + default:
> + die("should not happen, someone must be hit on the forehead");Show 17 quoted lines
> +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);Hm? (Note that I say UNSET, not SHORT... ;-)
Show 5 quoted lines
> + if (!rest)
> + continue;
> + if (*rest) {
> + if (*rest != '=')
> + continue;Is this really no error? For example, "git log --decorate-walls-and-roofs" would not fail...
> +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.
Show 5 quoted lines
> + 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.
> + 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).
In the same vein, OPT_COPY_DASHDASH should be named PARSE_OPT_KEEP_DASHDASH.
Show 7 quoted lines
> + 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.
Show 7 quoted lines
> + 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.)
Show 11 quoted lines
> 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?
Other than that: I like it very much.
Ciao, Dscho
Pierre Habouzit
