Re: [PATCH] Add a simple option parser.
- From
Alex Riesen <raa.lkml@gmail.com>
- Date
- Oct 13, 2007, 19:16 UTC
- Message-ID
- <20071013191655.GA2875@steel.home>
- In-Reply-To
- <1192282153-26684-2-git-send-email-madcoder@debian.org>
Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> +static int opterror(struct option *opt, const char *reason, int flags)
"const struct option *opt"? You never modify the struct option itself, only the values under the pointers it contains. Using const here will allow the compiler to reuse string constants (not that there will be much of the opportunity, but anyway) in the option arrays.
> +static int get_value(struct optparse_t *p, struct option *opt, int flags)
"const struct option *opt"?
> +static int parse_short_opt(struct optparse_t *p, struct option *options, int count)
"const struct option *options"?
> +int parse_options(int argc, const char **argv, > + struct option *options, int count, > + const char * const usagestr[], int flags)
"const struct option *options"?
> +void make_usage(const char * const usagestr[], struct option *opts, int cnt)
"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.
Show 8 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).
> + 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 :)