On Fri, 8 Aug 2014 12:59:50 -0700 Bill Spitzak <spit...@gmail.com> wrote:
> Fail on malformed numbers, such as --width=100mm Good. > Fail on = after booleans, such as --flag=false Okay. > Multiple single-letter booleans in one switch allowed, ie > -xyz is the same as -x -y -z. For wayland modules they all have > to belong to the same module. This was not accepted before, right? It is confusing that one needs to know that they are from the same module, so I'd rather just not accept combined single letter booleans at all. > Previous version could use text after the null at the end of > an argv entry. Now requires the = sign. What does this mean? I mean, how could one have ever used the old behaviour? Nothing comes to my mind, so I suppose that is ok. There are some style issues. First is the patch subject, it should be prefixed with the component the patch is touching. To get a feeling what the components are, look at the git-log of the file your are changing, and there should be a pattern. Though, I see that option-parser.c doesn't really have any pattern. Oh well, "shared:" should do then. More style issues below. > --- > shared/option-parser.c | 82 > +++++++++++++++++++++++++++++++----------------- > 1 file changed, 53 insertions(+), 29 deletions(-) > > diff --git a/shared/option-parser.c b/shared/option-parser.c > index c00349a..22bcf2f 100644 > --- a/shared/option-parser.c > +++ b/shared/option-parser.c > @@ -30,53 +30,77 @@ > > #include "config-parser.h" > > -static void > +static int > handle_option(const struct weston_option *option, char *value) > { > + char* p; Need empty line between declarations and code. > switch (option->type) { > case WESTON_OPTION_INTEGER: > - * (int32_t *) option->data = strtol(value, NULL, 0); > - return; > + * (int32_t *) option->data = strtol(value, &p, 0); > + return p > value && !*p; Took me a couple of times to read this return statement. How about: return *value != '\0' && *p == '\0'; Just a suggestion, doesn't matter. > case WESTON_OPTION_UNSIGNED_INTEGER: > - * (uint32_t *) option->data = strtoul(value, NULL, 0); > - return; > + * (uint32_t *) option->data = strtoul(value, &p, 0); > + return p > value && !*p; > case WESTON_OPTION_STRING: > * (char **) option->data = strdup(value); > - return; > - case WESTON_OPTION_BOOLEAN: > - * (int32_t *) option->data = 1; > - return; > + return 1; > default: > assert(0); > } > } > > +static int > +long_option(const struct weston_option *options, int count, char* arg) > +{ > + int k, len; Need empty line between declarations and code. > + for (k = 0; k < count; k++) { > + if (! options[k].name) continue; No space after in (! options[k].name). 'continue' should be on a new line > + len = strlen(options[k].name); > + if (strncmp(options[k].name, arg+2, len) != 0) continue; add spaces: arg + 2 'continue' should be on a new line > + if (options[k].type == WESTON_OPTION_BOOLEAN) { > + if (! arg[len+2]) { Extra space. > + * (int32_t *) options[k].data = 1; > + return 1; > + } > + } else if (arg[len+2] == '=') { spaces: len + 2 > + return handle_option(options+k, arg+len+3); spaces: options + k, arg + len + 3 > + } > + } > + return 0; > +} > + > +static int > +short_option(const struct weston_option *options, int count, char* arg) > +{ > + int i, k; Need empty line between declarations and code. > + for (i = 1; arg[i]; i++) { > + for (k = 0; ; k++) { > + if (k >= count) return 0; 'return' to a new line. > + if (options[k].short_name == arg[i]) break; 'break' to a new line. > + } > + if (options[k].type == WESTON_OPTION_BOOLEAN) > + * (int32_t *) options[k].data = 1; > + else > + return handle_option(options+k, arg+i+1); missing spaces > + } > + return arg[1]; /* error on just '-' */ Implicit cast to a "boolean" is kind of surprising, a comparison would be more obvious: return arg[1] != '\0'; Or better yet, just exit the function in the first lines, if arg[1] == '\0'. > +} > + > + > int > parse_options(const struct weston_option *options, > int count, int *argc, char *argv[]) > { > - int i, j, k, len = 0; > - > + int i, j; Need empty line between declarations and code. > for (i = 1, j = 1; i < *argc; i++) { > - for (k = 0; k < count; k++) { > - if (options[k].name) > - len = strlen(options[k].name); > - if (options[k].name && > - argv[i][0] == '-' && > - argv[i][1] == '-' && > - strncmp(options[k].name, &argv[i][2], len) == 0 && > - (argv[i][len + 2] == '=' || argv[i][len + 2] == > '\0')) { > - handle_option(&options[k], &argv[i][len + 3]); > - break; > - } else if (options[k].short_name && > - argv[i][0] == '-' && > - options[k].short_name == argv[i][1]) { > - handle_option(&options[k], &argv[i][2]); > - break; > - } > + if (argv[i][0] == '-') { > + if (argv[i][1] == '-') { > + if (long_option(options, count, argv[i])) > + continue; > + } else if (short_option(options, count, argv[i])) > + continue; > } > - if (k == count) > - argv[j++] = argv[i]; > + argv[j++] = argv[i]; > } > argv[j] = NULL; > *argc = j; But in general, the idea is good, IMHO. Thanks, pq _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel