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

[Bryce Harrington]

On Thu, Oct 22, 2015 at 04:25:45PM +0100, Auke Booij wrote:
> On 21 October 2015 at 19:13, Bryce Harrington <bryce at osg.samsung.com> wrote:
> > 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
> 
> I was about to fix all three points you brought up, when I realized
> there already is a precedence: allow-null. Its internal type in
> scanner.c can be found in struct arg; it is declared as "int
> nullable;". The corresponding error message is the same as for
> bitfield. It only allows lower-case "true" and "false".
> 
> So I did this in an attempt to be consistent. Now I'm not sure what to
> do, please advise.

Ah good find.  Probably should be changed there as well.  But there's
little point in further complicating this patch.  Leave it as true/false
here.  If it's worth supporting True/False there's no reason it can't be
done as a followup.

Bryce