Re: [PATCH] Add a simple option parser.
On Sat, Oct 13, 2007 at 02:39:10PM +0000, Johannes Schindelin wrote:
Show 8 quoted lines
> 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 13 quoted lines
> > +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.
Show 5 quoted lines
> > +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 25 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); 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... ;-)
Show 8 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... it would be an error, it will yield a "option not found".
Show 5 quoted lines
> > +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.
Show 8 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.Show 15 quoted lines
> > + 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 20 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.
>
> > + 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 15 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? 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.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
Pierre Habouzit <madcoder@debian.org>