Re: [PATCH] Add a simple option parser.
On Sat, Oct 13, 2007 at 07:16:55PM +0000, Alex Riesen wrote:
Show 10 quoted lines
> Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> > [...]
>
> "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.
Yes this has been spotted and fixed already.
Show 13 quoted lines
>
> > +{
> > + 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). Yes this is intentional, there should be at least on string in the
usagestr array.
Show 10 quoted lines
> > + 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 :)
the reason is that usage() is a wrapper around a callback, and I
suppose it's used by some GUI's or anything like that.
FWIW you can rework the .c like this:
pos = 0; /* and not pos = sb.len */
replace the strbuf_add* by the equivalents:
pos += printf("...."); and tada, you're done.
Note that in the most recent version, I also deal with a
OPTION_CALLBACK that passes the value to a callback.
Cheers,
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
Pierre Habouzit <madcoder@debian.org>