On Mon, Oct 26, 2015 at 06:37:28PM +0000, Auke Booij wrote:
> 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?
For now just this one patch would be more than fine.
One-off v+1's aren't usually a big deal.
The main thing we need next is I'd like to see a couple more R-b's,
particularly from folks who have expressed interest in this feature.
I'm fairly certain at this point we have solid consensus now but this'd
make it official.
> (Is there a systematic way to retract patches?)
Nope, kind of a fundamental limitation to doing change management via
mailing lists.
Bryce
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
