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

[Bryce Harrington]

On Mon, Oct 19, 2015 at 11:21:25PM +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.

Btw, thanks for pressing on with the work on this feature, despite the
difficulty finding common ground across the various programming
languages.
 
> 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.

>  };
>  
>  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?

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

> +
>  		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");
> +				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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel