From be6a5cdadeef4995cc935f2d2443f45f542ed125 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 5 Oct 2010 22:33:56 +1030 Subject: [PATCH] opt: minor API tweaks. Don't allow NULL descs, and put them in macro. Users can use "", but make sure it's explicit, not an accidental omission. Use errx() instead of assert() for API usage errors. Rename opt_flags to opt_type. --- ccan/opt/opt.c | 56 ++++++++++++++++++--------- ccan/opt/opt.h | 16 ++++---- ccan/opt/test/run-correct-reporting.c | 4 +- ccan/opt/test/run-helpers.c | 24 ++++++------ ccan/opt/test/run.c | 10 ++--- ccan/opt/test/utils.c | 20 +++++----- ccan/opt/usage.c | 27 ++++++------- 7 files changed, 86 insertions(+), 71 deletions(-) diff --git a/ccan/opt/opt.c b/ccan/opt/opt.c index e7ebc3dd..841e9b4e 100644 --- a/ccan/opt/opt.c +++ b/ccan/opt/opt.c @@ -32,7 +32,7 @@ static const char *next_name(const char *names, unsigned *len) static const char *first_opt(unsigned *i, unsigned *len) { for (*i = 0; *i < opt_count; (*i)++) { - if (opt_table[*i].flags == OPT_SUBTABLE) + if (opt_table[*i].type == OPT_SUBTABLE) continue; return first_name(opt_table[*i].names, len); } @@ -42,7 +42,7 @@ static const char *first_opt(unsigned *i, unsigned *len) static const char *next_opt(const char *p, unsigned *i, unsigned *len) { for (; *i < opt_count; (*i)++) { - if (opt_table[*i].flags == OPT_SUBTABLE) + if (opt_table[*i].type == OPT_SUBTABLE) continue; if (!p) return first_name(opt_table[*i].names, len); @@ -107,26 +107,44 @@ static void check_opt(const struct opt_table *entry) const char *p; unsigned len; - assert(entry->flags == OPT_HASARG || entry->flags == OPT_NOARG); + if (entry->type != OPT_HASARG && entry->type != OPT_NOARG) + errx(1, "Option %s: unknown entry type %u", + entry->names, entry->type); + + if (!entry->desc) + errx(1, "Option %s: description cannot be NULL", entry->names); + + + if (entry->names[0] != '-') + errx(1, "Option %s: does not begin with '-'", entry->names); - assert(entry->names[0] == '-'); for (p = first_name(entry->names, &len); p; p = next_name(p, &len)) { if (*p == '-') { - assert(len > 1); + if (len == 1) + errx(1, "Option %s: invalid long option '--'", + entry->names); opt_num_long++; } else { - assert(len == 1); - assert(*p != ':'); + if (len != 1) + errx(1, "Option %s: invalid short option" + " '%.*s'", entry->names, len+1, p-1); + if (*p == ':') + errx(1, "Option %s: invalid short option '-:'", + entry->names); opt_num_short++; - if (entry->flags == OPT_HASARG) { + if (entry->type == OPT_HASARG) { opt_num_short_arg++; - /* FIXME: -? with ops breaks getopt_long */ - assert(*p != '?'); + if (*p == '?') + errx(1, "Option %s: '-?' cannot take" + " an argument", entry->names); } } /* Don't document args unless there are some. */ - if (entry->flags == OPT_NOARG) - assert(p[len] != ' ' && p[len] != '='); + if (entry->type == OPT_NOARG) { + if (p[len] == ' ' || p[len] == '=') + errx(1, "Option %s: does not take arguments" + "'%s'", entry->names, p+len+1); + } } } @@ -136,7 +154,7 @@ static void add_opt(const struct opt_table *entry) opt_table[opt_count++] = *entry; } -void _opt_register(const char *names, enum opt_flags flags, +void _opt_register(const char *names, enum opt_type type, char *(*cb)(void *arg), char *(*cb_arg)(const char *optarg, void *arg), void (*show)(char buf[OPT_SHOW_LEN], const void *arg), @@ -144,7 +162,7 @@ void _opt_register(const char *names, enum opt_flags flags, { struct opt_table opt; opt.names = names; - opt.flags = flags; + opt.type = type; opt.cb = cb; opt.cb_arg = cb_arg; opt.show = show; @@ -162,8 +180,8 @@ void opt_register_table(const struct opt_table entry[], const char *desc) struct opt_table heading = OPT_SUBTABLE(NULL, desc); add_opt(&heading); } - for (i = 0; entry[i].flags != OPT_END; i++) { - if (entry[i].flags == OPT_SUBTABLE) + for (i = 0; entry[i].type != OPT_END; i++) { + if (entry[i].type == OPT_SUBTABLE) opt_register_table(subtable_of(&entry[i]), entry[i].desc); else { @@ -186,7 +204,7 @@ static char *make_optstring(void) str[num++] = ':'; for (p = first_sopt(&i); p; p = next_sopt(p, &i)) { str[num++] = *p; - if (opt_table[i].flags == OPT_HASARG) + if (opt_table[i].type == OPT_HASARG) str[num++] = ':'; } str[num++] = '\0'; @@ -205,7 +223,7 @@ static struct option *make_options(void) memcpy(buf, p, len); buf[len] = 0; options[num].name = buf; - options[num].has_arg = (opt_table[i].flags == OPT_HASARG); + options[num].has_arg = (opt_table[i].type == OPT_HASARG); options[num].flag = NULL; options[num].val = 0; num++; @@ -300,7 +318,7 @@ bool opt_parse(int *argc, char *argv[], void (*errlog)(const char *fmt, ...)) else e = find_long(longidx, &name); - if (e->flags == OPT_HASARG) + if (e->type == OPT_HASARG) problem = e->cb_arg(optarg, e->arg); else problem = e->cb(e->arg); diff --git a/ccan/opt/opt.h b/ccan/opt/opt.h index 65008091..1e25cce3 100644 --- a/ccan/opt/opt.h +++ b/ccan/opt/opt.h @@ -5,7 +5,7 @@ /* You can use this directly to build tables, but the macros will ensure * consistency and type safety. */ -enum opt_flags { +enum opt_type { OPT_NOARG = 1, /* -f/--foo */ OPT_HASARG = 2, /* -f arg/--foo=arg/--foo arg */ OPT_SUBTABLE = 4, /* Actually, longopt points to a subtable... */ @@ -17,7 +17,7 @@ enum opt_flags { struct opt_table { const char *names; /* slash-separated names, --longopt or -s */ - enum opt_flags flags; + enum opt_type type; char *(*cb)(void *arg); /* OPT_NOARG */ char *(*cb_arg)(const char *optarg, void *arg); /* OPT_HASARG */ void (*show)(char buf[OPT_SHOW_LEN], const void *arg); @@ -30,6 +30,7 @@ struct opt_table { * @names: the names of the option eg. "--foo", "-f" or "--foo/-f/--foobar". * @cb: the callback when the option is found. * @arg: the argument to hand to @cb. + * @desc: the description for opt_usage(), or opt_hidden. * * This is a typesafe wrapper for intializing a struct opt_table. The callback * of type "char *cb(type *)", "char *cb(const type *)" or "char *cb(void *)", @@ -46,8 +47,8 @@ struct opt_table { * See Also: * OPT_WITH_ARG() */ -#define OPT_WITHOUT_ARG(names, cb, arg) \ - (names), OPT_CB_NOARG((cb), (arg)) +#define OPT_WITHOUT_ARG(names, cb, arg, desc) \ + (names), OPT_CB_NOARG((cb), (arg)), (desc) /** * OPT_WITH_ARG() - macro for initializing long and short option (with arg) @@ -55,6 +56,7 @@ struct opt_table { * @cb: the callback when the option is found (along with ). * @show: the callback to print the value in get_usage (or NULL) * @arg: the argument to hand to @cb and @show + * @desc: the description for opt_usage(), or opt_hidden. * * This is a typesafe wrapper for intializing a struct opt_table. The callback * is of type "char *cb(const char *, type *)", @@ -80,8 +82,8 @@ struct opt_table { * See Also: * OPT_WITHOUT_ARG() */ -#define OPT_WITH_ARG(name, cb, show, arg) \ - (name), OPT_CB_ARG((cb), (show), (arg)) +#define OPT_WITH_ARG(name, cb, show, arg, desc) \ + (name), OPT_CB_ARG((cb), (show), (arg)), (desc) /** * OPT_SUBTABLE() - macro for including another table inside a table. @@ -286,7 +288,7 @@ char *opt_usage_and_exit(const char *extra); (arg) /* Non-typesafe register function. */ -void _opt_register(const char *names, enum opt_flags flags, +void _opt_register(const char *names, enum opt_type type, char *(*cb)(void *arg), char *(*cb_arg)(const char *optarg, void *arg), void (*show)(char buf[OPT_SHOW_LEN], const void *arg), diff --git a/ccan/opt/test/run-correct-reporting.c b/ccan/opt/test/run-correct-reporting.c index 44f68ae2..bbac3402 100644 --- a/ccan/opt/test/run-correct-reporting.c +++ b/ccan/opt/test/run-correct-reporting.c @@ -11,7 +11,7 @@ int main(int argc, char *argv[]) plan_tests(12); /* --aaa without args. */ - opt_register_arg("-a/--aaa", test_arg, NULL, "aaa", NULL); + opt_register_arg("-a/--aaa", test_arg, NULL, "aaa", ""); ok1(!parse_args(&argc, &argv, "--aaa", NULL)); ok1(strstr(err_output, ": --aaa: option requires an argument")); free(err_output); @@ -22,7 +22,7 @@ int main(int argc, char *argv[]) err_output = NULL; /* Multiple */ - opt_register_arg("--bbb/-b/-c/--ccc", test_arg, NULL, "aaa", NULL); + opt_register_arg("--bbb/-b/-c/--ccc", test_arg, NULL, "aaa", ""); ok1(!parse_args(&argc, &argv, "--bbb", NULL)); ok1(strstr(err_output, ": --bbb: option requires an argument")); free(err_output); diff --git a/ccan/opt/test/run-helpers.c b/ccan/opt/test/run-helpers.c index 48b7cd8b..91e66374 100644 --- a/ccan/opt/test/run-helpers.c +++ b/ccan/opt/test/run-helpers.c @@ -55,10 +55,10 @@ int main(int argc, char *argv[]) { bool arg = false; reset_options(); - opt_register_noarg("-a", opt_set_bool, &arg, NULL); + opt_register_noarg("-a", opt_set_bool, &arg, ""); ok1(parse_args(&argc, &argv, "-a", NULL)); ok1(arg); - opt_register_arg("-b", opt_set_bool_arg, NULL, &arg, NULL); + opt_register_arg("-b", opt_set_bool_arg, NULL, &arg, ""); ok1(parse_args(&argc, &argv, "-b", "no", NULL)); ok1(!arg); ok1(parse_args(&argc, &argv, "-b", "yes", NULL)); @@ -75,11 +75,11 @@ int main(int argc, char *argv[]) { bool arg = true; reset_options(); - opt_register_noarg("-a", opt_set_invbool, &arg, NULL); + opt_register_noarg("-a", opt_set_invbool, &arg, ""); ok1(parse_args(&argc, &argv, "-a", NULL)); ok1(!arg); opt_register_arg("-b", opt_set_invbool_arg, NULL, - &arg, NULL); + &arg, ""); ok1(parse_args(&argc, &argv, "-b", "no", NULL)); ok1(arg); ok1(parse_args(&argc, &argv, "-b", "yes", NULL)); @@ -96,7 +96,7 @@ int main(int argc, char *argv[]) { char *arg = (char *)"wrong"; reset_options(); - opt_register_arg("-a", opt_set_charp, NULL, &arg, NULL); + opt_register_arg("-a", opt_set_charp, NULL, &arg, "All"); ok1(parse_args(&argc, &argv, "-a", "string", NULL)); ok1(strcmp(arg, "string") == 0); } @@ -104,7 +104,7 @@ int main(int argc, char *argv[]) { int arg = 1000; reset_options(); - opt_register_arg("-a", opt_set_intval, NULL, &arg, NULL); + opt_register_arg("-a", opt_set_intval, NULL, &arg, "All"); ok1(parse_args(&argc, &argv, "-a", "9999", NULL)); ok1(arg == 9999); ok1(parse_args(&argc, &argv, "-a", "-9999", NULL)); @@ -121,7 +121,7 @@ int main(int argc, char *argv[]) { unsigned int arg = 1000; reset_options(); - opt_register_arg("-a", opt_set_uintval, NULL, &arg, NULL); + opt_register_arg("-a", opt_set_uintval, NULL, &arg, "All"); ok1(parse_args(&argc, &argv, "-a", "9999", NULL)); ok1(arg == 9999); ok1(!parse_args(&argc, &argv, "-a", "-9999", NULL)); @@ -145,7 +145,7 @@ int main(int argc, char *argv[]) { long int arg = 1000; reset_options(); - opt_register_arg("-a", opt_set_longval, NULL, &arg, NULL); + opt_register_arg("-a", opt_set_longval, NULL, &arg, "All"); ok1(parse_args(&argc, &argv, "-a", "9999", NULL)); ok1(arg == 9999); ok1(parse_args(&argc, &argv, "-a", "-9999", NULL)); @@ -164,7 +164,7 @@ int main(int argc, char *argv[]) { unsigned long int arg = 1000; reset_options(); - opt_register_arg("-a", opt_set_ulongval, NULL, &arg, NULL); + opt_register_arg("-a", opt_set_ulongval, NULL, &arg, "All"); ok1(parse_args(&argc, &argv, "-a", "9999", NULL)); ok1(arg == 9999); ok1(!parse_args(&argc, &argv, "-a", "-9999", NULL)); @@ -182,7 +182,7 @@ int main(int argc, char *argv[]) { int arg = 1000; reset_options(); - opt_register_noarg("-a", opt_inc_intval, &arg, NULL); + opt_register_noarg("-a", opt_inc_intval, &arg, ""); ok1(parse_args(&argc, &argv, "-a", NULL)); ok1(arg == 1001); ok1(parse_args(&argc, &argv, "-a", "-a", NULL)); @@ -196,7 +196,7 @@ int main(int argc, char *argv[]) int exitval; reset_options(); opt_register_noarg("-a", - opt_version_and_exit, "1.2.3", NULL); + opt_version_and_exit, "1.2.3", ""); exitval = setjmp(exited); if (exitval == 0) { parse_args(&argc, &argv, "-a", NULL); @@ -214,7 +214,7 @@ int main(int argc, char *argv[]) int exitval; reset_options(); opt_register_noarg("-a", - opt_usage_and_exit, "[args]", NULL); + opt_usage_and_exit, "[args]", ""); exitval = setjmp(exited); if (exitval == 0) { parse_args(&argc, &argv, "-a", NULL); diff --git a/ccan/opt/test/run.c b/ccan/opt/test/run.c index 4eb78c93..82c0398c 100644 --- a/ccan/opt/test/run.c +++ b/ccan/opt/test/run.c @@ -20,7 +20,7 @@ int main(int argc, char *argv[]) plan_tests(148); /* Simple short arg.*/ - opt_register_noarg("-a", test_noarg, NULL, NULL); + opt_register_noarg("-a", test_noarg, NULL, "All"); ok1(parse_args(&argc, &argv, "-a", NULL)); ok1(argc == 1); ok1(argv[0] == myname); @@ -28,7 +28,7 @@ int main(int argc, char *argv[]) ok1(test_cb_called == 1); /* Simple long arg. */ - opt_register_noarg("--aaa", test_noarg, NULL, NULL); + opt_register_noarg("--aaa", test_noarg, NULL, "AAAAll"); ok1(parse_args(&argc, &argv, "--aaa", NULL)); ok1(argc == 1); ok1(argv[0] == myname); @@ -36,7 +36,7 @@ int main(int argc, char *argv[]) ok1(test_cb_called == 2); /* Both long and short args. */ - opt_register_noarg("--aaa/-a", test_noarg, NULL, NULL); + opt_register_noarg("--aaa/-a", test_noarg, NULL, "AAAAAAll"); ok1(parse_args(&argc, &argv, "--aaa", "-a", NULL)); ok1(argc == 1); ok1(argv[0] == myname); @@ -54,7 +54,7 @@ int main(int argc, char *argv[]) /* Argument variants. */ reset_options(); test_cb_called = 0; - opt_register_arg("-a/--aaa", test_arg, NULL, "aaa", NULL); + opt_register_arg("-a/--aaa", test_arg, NULL, "aaa", "AAAAAAll"); ok1(parse_args(&argc, &argv, "--aaa", "aaa", NULL)); ok1(argc == 1); ok1(argv[0] == myname); @@ -201,7 +201,7 @@ int main(int argc, char *argv[]) reset_options(); /* glibc's getopt does not handle ? with arguments. */ - opt_register_noarg("-?", test_noarg, NULL, NULL); + opt_register_noarg("-?", test_noarg, NULL, "Help"); ok1(parse_args(&argc, &argv, "-?", NULL)); ok1(test_cb_called == 1); ok1(parse_args(&argc, &argv, "-a", NULL) == false); diff --git a/ccan/opt/test/utils.c b/ccan/opt/test/utils.c index bafb6743..4e5b4d77 100644 --- a/ccan/opt/test/utils.c +++ b/ccan/opt/test/utils.c @@ -70,33 +70,33 @@ bool parse_args(int *argc, char ***argv, ...) struct opt_table short_table[] = { /* Short opts, different args. */ - { OPT_WITHOUT_ARG("-a", test_noarg, "a"), "Description of a" }, - { OPT_WITH_ARG("-b", test_arg, show_arg, "b"), "Description of b" }, + { OPT_WITHOUT_ARG("-a", test_noarg, "a", "Description of a") }, + { OPT_WITH_ARG("-b", test_arg, show_arg, "b", "Description of b") }, OPT_ENDTABLE }; struct opt_table long_table[] = { /* Long opts, different args. */ - { OPT_WITHOUT_ARG("--ddd", test_noarg, "ddd"), "Description of ddd" }, - { OPT_WITH_ARG("--eee ", test_arg, show_arg, "eee"), }, + { OPT_WITHOUT_ARG("--ddd", test_noarg, "ddd", "Description of ddd") }, + { OPT_WITH_ARG("--eee ", test_arg, show_arg, "eee", "") }, OPT_ENDTABLE }; struct opt_table long_and_short_table[] = { /* Short and long, different args. */ - { OPT_WITHOUT_ARG("--ggg/-g", test_noarg, "ggg"), - "Description of ggg" }, - { OPT_WITH_ARG("-h/--hhh", test_arg, NULL, "hhh"), - "Description of hhh"}, + { OPT_WITHOUT_ARG("--ggg/-g", test_noarg, "ggg", + "Description of ggg") }, + { OPT_WITH_ARG("-h/--hhh", test_arg, NULL, "hhh", + "Description of hhh") }, OPT_ENDTABLE }; /* Sub-table test. */ struct opt_table subtables[] = { /* Two short, and two long long, no description */ - { OPT_WITH_ARG("--jjj/-j/--lll/-l", test_arg, show_arg, "jjj") }, + { OPT_WITH_ARG("--jjj/-j/--lll/-l", test_arg, show_arg, "jjj", "") }, /* Hidden option */ - { OPT_WITH_ARG("--mmm/-m", test_arg, show_arg, "mmm"), opt_hidden }, + { OPT_WITH_ARG("--mmm/-m", test_arg, show_arg, "mmm", opt_hidden) }, OPT_SUBTABLE(short_table, NULL), OPT_SUBTABLE(long_table, "long table options"), OPT_SUBTABLE(long_and_short_table, NULL), diff --git a/ccan/opt/usage.c b/ccan/opt/usage.c index 25678d66..b7381bc7 100644 --- a/ccan/opt/usage.c +++ b/ccan/opt/usage.c @@ -35,15 +35,13 @@ char *opt_usage(const char *argv0, const char *extra) + strlen("\n"); for (i = 0; i < opt_count; i++) { - if (opt_table[i].flags == OPT_SUBTABLE) { + if (opt_table[i].type == OPT_SUBTABLE) { len += strlen("\n") + strlen(opt_table[i].desc) + strlen(":\n"); } else if (opt_table[i].desc != opt_hidden) { len += strlen(opt_table[i].names) + strlen(" "); - if (opt_table[i].desc) { - len += strlen(OPT_SPACE_PAD) - + strlen(opt_table[i].desc) + 1; - } + len += strlen(OPT_SPACE_PAD) + + strlen(opt_table[i].desc) + 1; if (opt_table[i].show) { len += strlen("(default: %s)") + OPT_SHOW_LEN + sizeof("..."); @@ -73,29 +71,26 @@ char *opt_usage(const char *argv0, const char *extra) for (i = 0; i < opt_count; i++) { if (opt_table[i].desc == opt_hidden) continue; - if (opt_table[i].flags == OPT_SUBTABLE) { + if (opt_table[i].type == OPT_SUBTABLE) { p += sprintf(p, "%s:\n", opt_table[i].desc); continue; } len = sprintf(p, "%s", opt_table[i].names); - if (opt_table[i].flags == OPT_HASARG + if (opt_table[i].type == OPT_HASARG && !strchr(opt_table[i].names, ' ') && !strchr(opt_table[i].names, '=')) len += sprintf(p + len, " "); - if (opt_table[i].desc || opt_table[i].show) - len += sprintf(p + len, "%.*s", - len < strlen(OPT_SPACE_PAD) - ? strlen(OPT_SPACE_PAD) - len : 1, - OPT_SPACE_PAD); + len += sprintf(p + len, "%.*s", + len < strlen(OPT_SPACE_PAD) + ? strlen(OPT_SPACE_PAD) - len : 1, + OPT_SPACE_PAD); - if (opt_table[i].desc) - len += sprintf(p + len, "%s", opt_table[i].desc); + len += sprintf(p + len, "%s", opt_table[i].desc); if (opt_table[i].show) { char buf[OPT_SHOW_LEN + sizeof("...")]; strcpy(buf + OPT_SHOW_LEN, "..."); opt_table[i].show(buf, opt_table[i].arg); - len += sprintf(p + len, "%s(default: %s)", - opt_table[i].desc ? " " : "", buf); + len += sprintf(p + len, " (default: %s)", buf); } p += len; p += sprintf(p, "\n"); -- 2.39.2