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

[Auke Booij]

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.)

>> +             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.