On 26 October 2015 at 18:07, Bryce Harrington <br...@osg.samsung.com> wrote: > On Sat, Oct 24, 2015 at 12:07:49PM +0100, Auke Booij wrote: >> The scanner now checks whether arguments that have an associated >> <enum> have the right type. >> An argument with an enum attribute must be of type int or uint, >> and if the <enum> with that name has the bitfield attribute >> set to true, then the argument must be of type uint. >> >> Signed-off-by: Auke Booij <a...@tulcod.com> > > Reviewed-by: Bryce Harrington <br...@osg.samsung.com> > > A couple really minor nits below, not really worth doing unless you need > to do another rev of this patch for some other reason. > >> --- >> src/scanner.c | 70 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 70 insertions(+) >> >> diff --git a/src/scanner.c b/src/scanner.c >> index f456aa5..9856475 100644 >> --- a/src/scanner.c >> +++ b/src/scanner.c >> @@ -128,6 +128,7 @@ struct arg { >> char *interface_name; >> struct wl_list link; >> char *summary; >> + char *enumeration_name; >> }; >> >> struct enumeration { >> @@ -136,6 +137,7 @@ struct enumeration { >> struct wl_list entry_list; >> struct wl_list link; >> struct description *description; >> + int bitfield; > > This appears to be used for tracking only a yes/no type value, so maybe > consider making it a boolean? > >> }; >> >> struct entry { >> @@ -540,6 +542,8 @@ start_element(void *data, const char *element_name, >> const char **atts) >> const char *summary = NULL; >> const char *since = NULL; >> const char *allow_null = NULL; >> + const char *enumeration_name = NULL; >> + const char *bitfield = NULL; >> int i, version = 0; >> >> ctx->loc.line_number = XML_GetCurrentLineNumber(ctx->parser); >> @@ -562,6 +566,10 @@ start_element(void *data, const char *element_name, >> const char **atts) >> since = atts[i + 1]; >> if (strcmp(atts[i], "allow-null") == 0) >> allow_null = atts[i + 1]; >> + if (strcmp(atts[i], "enum") == 0) >> + enumeration_name = atts[i + 1]; >> + if (strcmp(atts[i], "bitfield") == 0) >> + bitfield = atts[i + 1]; >> } >> >> ctx->character_data_length = 0; >> @@ -655,6 +663,14 @@ start_element(void *data, const char *element_name, >> const char **atts) >> "allow-null is only valid for objects, >> strings, and arrays"); >> } >> >> + if (enumeration_name == NULL || strcmp(enumeration_name, "") >> == 0) >> + arg->enumeration_name = NULL; >> + else >> + arg->enumeration_name = xstrdup(enumeration_name); >> + >> + if (allow_null != NULL && !is_nullable_type(arg)) >> + fail(&ctx->loc, "allow-null is only valid for objects, >> strings, and arrays"); >> + >> if (summary) >> arg->summary = xstrdup(summary); >> >> @@ -665,6 +681,14 @@ start_element(void *data, const char *element_name, >> const char **atts) >> fail(&ctx->loc, "no enum name given"); >> >> enumeration = create_enumeration(name); >> + >> + if (bitfield == NULL || strcmp(bitfield, "false") == 0) >> + enumeration->bitfield = 0; >> + else if (strcmp(bitfield, "true") == 0) >> + enumeration->bitfield =1; > > Space needed after the = > >> + else >> + fail(&ctx->loc, "invalid value for bitfield attribute >> (%s)", bitfield); >> + >> wl_list_insert(ctx->interface->enumeration_list.prev, >> &enumeration->link); >> >> @@ -701,6 +725,46 @@ start_element(void *data, const char *element_name, >> const char **atts) >> } >> >> static void >> +verify_arguments(struct parse_context *ctx, struct wl_list *messages, >> struct wl_list *enumerations) >> +{ >> + struct message *m; >> + wl_list_for_each(m, messages, link) { >> + struct arg *a; >> + wl_list_for_each(a, &m->arg_list, link) { >> + struct enumeration *e, *f; >> + >> + if (!a->enumeration_name) >> + continue; >> + >> + f = NULL; >> + wl_list_for_each(e, enumerations, link) { >> + if(strcmp(e->name, a->enumeration_name) == 0) >> + f = e; >> + } >> + >> + if (f == NULL) >> + fail(&ctx->loc, >> + "could not find enumeration %s", >> + a->enumeration_name); >> + >> + switch (a->type) { >> + case INT: >> + if (f->bitfield) >> + fail(&ctx->loc, >> + "bitfield-style enum must be >> referenced by uint"); > > I think maybe you mean "must only be referenced"? > >> + break; >> + case UNSIGNED: >> + break; >> + default: >> + fail(&ctx->loc, >> + "enumeration-style argument has wrong >> type"); >> + } >> + } >> + } >> + >> +} >> + >> +static void >> end_element(void *data, const XML_Char *name) >> { >> struct parse_context *ctx = data; >> @@ -723,6 +787,12 @@ end_element(void *data, const XML_Char *name) >> ctx->enumeration->name); >> } >> ctx->enumeration = NULL; >> + } else if (strcmp(name, "interface") == 0) { >> + struct interface *i = ctx->interface; >> + >> + verify_arguments(ctx, &i->request_list, &i->enumeration_list); >> + verify_arguments(ctx, &i->event_list, &i->enumeration_list); >> + >> } >> } >> >> -- >> 2.6.1 >> >> _______________________________________________ >> wayland-devel mailing list >> wayland-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
I was actually somewhat planning to fix these things (for both bitfield and allow-null) in after this patch is merged: I didn't want to include unrelated fixes into this series, but also wanted to write code in a consistent style. So that's why it is still like this. (except for the space after = of course, which I missed) But now I noticed that my patch introduces a useless check regarding allow_null on line 672 (this is already checked earlier, and is there because I rebased incorrectly at some point) - this should NOT be in here! I will fix all of this. Should I send in a new series, or can I just update this one patch? (Is there a systematic way to retract patches?) _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel