Re: [PATCH 5 of 5] xl: Introduce helper macro for option parsing

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2012-10-17 at 17:13 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH 5 of 5] xl: Introduce helper macro for option parsing"):
> > xl: Introduce helper macro for option parsing.
> 
> The idea is a reasonable one, but I have some quibbles.
> 
> > +#define FOREACH_OPT(_opt, _opts, _lopts, _help, _req)           \
> > +    while (((_opt) = def_getopt(argc, argv, (_opts), (_lopts),  \
> > +                                (_help), (_req))) != -1)        \
> > +        switch (opt)
> 
> This macro name should have the word SWITCH in it too so you know it
> has to take cases and that "break" doesn't do what you think.
> FOREACH_SWITCH_OPT maybe.

Good idea.

> 
> > @@ -2304,8 +2309,10 @@ int main_memmax(int argc, char **argv)
> >      char *mem;
> >      int rc;
> > 
> > -    if ((opt = def_getopt(argc, argv, "", NULL, "mem-max", 2)) != -1)
> > +    FOREACH_OPT(opt, "", NULL, "mem-max", 2) {
> > +    case 0: case 2:
> >          return opt;
> > +    }
> 
> This error handling behaviour (?) is rather opaque.  I RTFM
> getopt_long and AFAICT it returns 0 only for certain long options and
> it doesn't seem to mention returning 2.

2 comes from our existing implementation of def_getopt, which returns it
in the "not enough args" case. I don't think any callers differentiate
between that and 0 which is the other error indication though.

> Perhaps this would all be clearer if FOREACH_OPT had a big comment
> explaining how to use it.

Yes, will do.