Re: [PATCH] Add a simple option parser.

[Pierre Habouzit <madcoder@debian.org>]

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

On Sat, Oct 13, 2007 at 07:16:55PM +0000, Alex Riesen wrote:
> 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.

  Ok.

> 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.

> 
> > +{
> > +	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.


> > +     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

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