[PATCH v2 3/4] scanner: enforce correct argument type for enums

[Bryce Harrington]

On Wed, Oct 21, 2015 at 02:23:53PM +0100, Auke Booij wrote:
> On 20 October 2015 at 08:57, Bryce Harrington <bryce at osg.samsung.com> wrote:
> > On Mon, Oct 19, 2015 at 11:21:25PM +0100, Auke Booij wrote:
> >> Signed-off-by: Auke Booij <auke at tulcod.com>
> >> ---
> >>  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;
> >
> > Looks like this code only sets this to 0 or 1; may as well make this bool.
> > Other places in scanner.c use bool, so seems to be perfectly allowable.
> 
> Sure, I will change this.
> 
> >>  };
> >>
> >>  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;
> >
> > Would it be worth allowing True/False as well?
> 
> Well this might be worth thinking about a bit more. I don't think
> there are any boolean fields in other wayland configuration (correct
> me if I'm wrong), so whatever we choose, it will set a precedence.
> Should I also allow 0/1? Yes/No? (Xorg seems to be rather lax in all
> this.)

My rationale here is that some programming languages (e.g. Python) use
the capitalized version, so especially as this feature is described as
targeting language binders, it might remove a tiny irritation for folks
who have it in finger muscle memory.  A very small thing but it costs us
little.

>From this rationale, Yes/No may be superfluous, as I don't think there's
any programming languages that require those terms for booleans.  Seems
little harm, but perhaps this is another thing that'd be easier to add
than to remove so maybe wait until we spot a use for it in practice?

0/1 is a good idea, but I know there are enough situations in the wild
where the meanings are reversed that disallowing it might actually
prevent some rare but difficult to diagnose bugs.  Similarly, supporting
numbers for this type of field might circumvent various languages
typechecking abilities; probably not a big deal in this particular case
but like you say, we would be wise to consider precidence setting...

> >> +             else
> >> +                     fail(&ctx->loc, "invalid value for bitfield attribute (%s)", bitfield);
> >
> > May as well say here that only true/false are accepted in bitfields.
> > Might save anyone that hits this from a trip to the documentation.
> 
> I will fix this.

Bryce