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

[Auke Booij]

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.