[PATCH V2] wayland.xml: add "enum", "bitfield" and "is_bitfield" attributes

Pekka Paalanen ppaalanen at gmail.com
Mon Sep 22 02:21:11 PDT 2014


On Wed, 3 Sep 2014 19:44:04 +0200
"Nils Chr. Brause" <nilschrbrause at gmail.com> wrote:

> This replaces "[PATCH wayland] Add "enum" attribute to "arg" elements".
> Previous concers have be incorporated and it is meant for 1.7.
> 
> From 733fb18b163de93276f092a4be100c7e0c0c723f Mon Sep 17 00:00:00 2001
> From: "Nils Chr. Brause" <nilschrbrause at googlemail.com>
> Date: Wed, 3 Sep 2014 19:18:28 +0200
> Subject: [PATCH] wayland.xml: add "enum", "bitfield" and "is_bitfield"
>  attributes
> 
> There are programming languages, that are more strongly typed than
> C. People creating Wayland bindings for these languages will want to
> make use of this strong type system by declaring each enumeration a
> distinct type and differentiating between enumerations and bit fields.
> 
> For code generation to work with these languages, there needs to be
> information about which enumerations are actually bit fields and which
> enumerations or bit fields may be passed/received in which
> request/event arguments.
> 
> This is accomplished by adding an "is_bitfield" attribute to "enum"
> elements, which are actually bitfields and by adding "enum" or
> "bitfield" attributes to the "arg" elements of requests and
> events. The values of the "enum" and "bitfield" attributes have the
> format "$interface.$name", where "$interface" is the name of the
> interface, where the enumeration or bit field is declared and "$name"
> is the name of the enumeration or bit field, which is used in this
> argument.
> 
> The scanner has been modified to enforce those rules.
> ---
>  protocol/wayland.dtd |   3 ++
>  protocol/wayland.xml |  42 ++++++++++-----------
>  src/scanner.c        | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 124 insertions(+), 22 deletions(-)
> 
> diff --git a/protocol/wayland.dtd b/protocol/wayland.dtd
> index b8b1573..4506201 100644
> --- a/protocol/wayland.dtd
> +++ b/protocol/wayland.dtd
> @@ -14,6 +14,7 @@
>  <!ELEMENT enum (description?,entry*)>
>    <!ATTLIST enum name CDATA #REQUIRED>
>    <!ATTLIST enum since CDATA #IMPLIED>
> +  <!ATTLIST enum is_bitfield CDATA #IMPLIED>
>  <!ELEMENT entry (description?)>
>    <!ATTLIST entry name CDATA #REQUIRED>
>    <!ATTLIST entry value CDATA #REQUIRED>
> @@ -25,5 +26,7 @@
>    <!ATTLIST arg summary CDATA #IMPLIED>
>    <!ATTLIST arg interface CDATA #IMPLIED>
>    <!ATTLIST arg allow-null CDATA #IMPLIED>
> +  <!ATTLIST arg enum CDATA #IMPLIED>
> +  <!ATTLIST arg bitfield CDATA #IMPLIED>
>  <!ELEMENT description (#PCDATA)>
>    <!ATTLIST description summary CDATA #REQUIRED>
> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index bb457bc..878c347 100644
> --- a/protocol/wayland.xml
> +++ b/protocol/wayland.xml
> @@ -229,7 +229,7 @@
>        <arg name="width" type="int"/>
>        <arg name="height" type="int"/>
>        <arg name="stride" type="int"/>
> -      <arg name="format" type="uint"/>
> +      <arg name="format" type="uint" enum="wl_shm.format"/>
>      </request>
>  
>      <request name="destroy" type="destructor">
> @@ -367,7 +367,7 @@
>  	can be used for buffers. Known formats include
>  	argb8888 and xrgb8888.
>        </description>
> -      <arg name="format" type="uint"/>
> +      <arg name="format" type="uint" enum="wl_shm.format"/>
>      </event>
>    </interface>
>  
> @@ -723,7 +723,7 @@
>        <arg name="serial" type="uint" summary="serial of the implicit grab on the pointer"/>
>      </request>
>  
> -    <enum name="resize">
> +    <enum name="resize" is_bitfield="true">
>        <description summary="edge values for resizing">
>  	These values are used to indicate which edge of a surface
>  	is being dragged in a resize operation. The server may
> @@ -751,7 +751,7 @@
>        </description>
>        <arg name="seat" type="object" interface="wl_seat" summary="the wl_seat whose pointer is used"/>
>        <arg name="serial" type="uint" summary="serial of the implicit grab on the pointer"/>
> -      <arg name="edges" type="uint" summary="which edge or corner is being dragged"/>
> +      <arg name="edges" type="uint" summary="which edge or corner is being dragged" bitfield="wl_shell_surface.resize"/>
>      </request>
>  
>      <request name="set_toplevel">
> @@ -762,7 +762,7 @@
>        </description>
>      </request>
>  
> -    <enum name="transient">
> +    <enum name="transient" is_bitfield="true">
>        <description summary="details of transient behaviour">
>  	These flags specify details of the expected behaviour
>  	of transient surfaces. Used in the set_transient request.
> @@ -784,7 +784,7 @@
>        <arg name="parent" type="object" interface="wl_surface"/>
>        <arg name="x" type="int"/>
>        <arg name="y" type="int"/>
> -      <arg name="flags" type="uint"/>
> +      <arg name="flags" type="uint" bitfield="wl_shell_surface.transient"/>
>      </request>
>  
>      <enum name="fullscreen_method">
> @@ -835,7 +835,7 @@
>  	with the dimensions for the output on which the surface will
>  	be made fullscreen.
>        </description>
> -      <arg name="method" type="uint"/>
> +      <arg name="method" type="uint" enum="wl_shell_surface.fullscreen_method"/>
>        <arg name="framerate" type="uint"/>
>        <arg name="output" type="object" interface="wl_output" allow-null="true"/>
>      </request>
> @@ -868,7 +868,7 @@
>        <arg name="parent" type="object" interface="wl_surface"/>
>        <arg name="x" type="int"/>
>        <arg name="y" type="int"/>
> -      <arg name="flags" type="uint"/>
> +      <arg name="flags" type="uint" bitfield="wl_shell_surface.transient"/>
>      </request>
>  
>      <request name="set_maximized">
> @@ -949,7 +949,7 @@
>  	in surface local coordinates.
>        </description>
>  
> -      <arg name="edges" type="uint"/>
> +      <arg name="edges" type="uint" bitfield="wl_shell_surface.resize"/>
>        <arg name="width" type="int"/>
>        <arg name="height" type="int"/>
>      </event>
> @@ -1242,7 +1242,7 @@
>  	wl_output.transform enum the invalid_transform protocol error
>  	is raised.
>        </description>
> -      <arg name="transform" type="int"/>
> +      <arg name="transform" type="int" bitfield="wl_output.transform"/>
>      </request>
>  
>      <!-- Version 3 additions -->
> @@ -1285,7 +1285,7 @@
>        maintains a keyboard focus and a pointer focus.
>      </description>
>  
> -    <enum name="capability">
> +    <enum name="capability" is_bitfield="true">
>        <description summary="seat capability bitmask">
>          This is a bitmask of capabilities this seat has; if a member is
>          set, then it is present on the seat.
> @@ -1301,7 +1301,7 @@
>  	keyboard or touch capabilities.  The argument is a capability
>  	enum containing the complete set of capabilities this seat has.
>        </description>
> -      <arg name="capabilities" type="uint"/>
> +      <arg name="capabilities" type="uint" bitfield="wl_seat.capability"/>
>      </event>
>  
>      <request name="get_pointer">
> @@ -1461,7 +1461,7 @@
>        <arg name="serial" type="uint"/>
>        <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
>        <arg name="button" type="uint"/>
> -      <arg name="state" type="uint"/>
> +      <arg name="state" type="uint" enum="wl_pointer.button_state"/>
>      </event>
>  
>      <enum name="axis">
> @@ -1493,7 +1493,7 @@
>        </description>
>  
>        <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
> -      <arg name="axis" type="uint"/>
> +      <arg name="axis" type="uint" enum="wl_pointer.axis"/>
>        <arg name="value" type="fixed"/>
>      </event>
>  
> @@ -1527,7 +1527,7 @@
>  	This event provides a file descriptor to the client which can be
>  	memory-mapped to provide a keyboard mapping description.
>        </description>
> -      <arg name="format" type="uint"/>
> +      <arg name="format" type="uint" enum="wl_keyboard.keymap_format"/>
>        <arg name="fd" type="fd"/>
>        <arg name="size" type="uint"/>
>      </event>
> @@ -1572,7 +1572,7 @@
>        <arg name="serial" type="uint"/>
>        <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
>        <arg name="key" type="uint"/>
> -      <arg name="state" type="uint"/>
> +      <arg name="state" type="uint" enum="wl_keyboard.key_state"/>
>      </event>
>  
>      <event name="modifiers">
> @@ -1714,7 +1714,7 @@
>        <entry name="vertical_bgr" value="5"/>
>      </enum>
>  
> -    <enum name="transform">
> +    <enum name="transform" is_bitfield="true">
>        <description summary="transform from framebuffer to output">
>  	This describes the transform that a compositor will apply to a
>  	surface to compensate for the rotation or mirroring of an
> @@ -1753,17 +1753,17 @@
>  	   summary="width in millimeters of the output"/>
>        <arg name="physical_height" type="int"
>  	   summary="height in millimeters of the output"/>
> -      <arg name="subpixel" type="int"
> +      <arg name="subpixel" type="int" enum="wl_output.subpixel"
>  	   summary="subpixel orientation of the output"/>
>        <arg name="make" type="string"
>  	   summary="textual description of the manufacturer"/>
>        <arg name="model" type="string"
>  	   summary="textual description of the model"/>
> -      <arg name="transform" type="int"
> +      <arg name="transform" type="int" bitfield="wl_output.transform"
>  	   summary="transform that maps framebuffer to output"/>
>      </event>
>  
> -    <enum name="mode">
> +    <enum name="mode" is_bitfield="true">
>        <description summary="mode information">
>  	These flags describe properties of an output mode.
>  	They are used in the flags bitfield of the mode event.
> @@ -1790,7 +1790,7 @@
>          the output may be scaled, as described in wl_output.scale,
>          or transformed , as described in wl_output.transform.
>        </description>
> -      <arg name="flags" type="uint" summary="bitfield of mode flags"/>
> +      <arg name="flags" type="uint" summary="bitfield of mode flags" bitfield="wl_output.mode"/>
>        <arg name="width" type="int" summary="width of the mode in hardware units"/>
>        <arg name="height" type="int" summary="height of the mode in hardware units"/>
>        <arg name="refresh" type="int" summary="vertical refresh rate in mHz"/>
> diff --git a/src/scanner.c b/src/scanner.c
> index 72fd3e8..b240790 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -112,6 +112,8 @@ struct arg {
>  	enum arg_type type;
>  	int nullable;
>  	char *interface_name;
> +	char *enum_name;
> +	char *bitfield_name;
>  	struct wl_list link;
>  	char *summary;
>  };
> @@ -119,6 +121,7 @@ struct arg {
>  struct enumeration {
>  	char *name;
>  	char *uppercase_name;
> +	int is_bitfield;
>  	struct wl_list entry_list;
>  	struct wl_list link;
>  	struct description *description;
> @@ -314,7 +317,7 @@ start_element(void *data, const char *element_name, const char **atts)
>  	struct entry *entry;
>  	struct description *description;
>  	const char *name, *type, *interface_name, *value, *summary, *since;
> -	const char *allow_null;
> +	const char *allow_null, *enum_name, *bitfield_name, *is_bitfield;
>  	char *end;
>  	int i, version;
>  
> @@ -323,11 +326,14 @@ start_element(void *data, const char *element_name, const char **atts)
>  	type = NULL;
>  	version = 0;
>  	interface_name = NULL;
> +	enum_name = NULL;
> +	bitfield_name = NULL;
>  	value = NULL;
>  	summary = NULL;
>  	description = NULL;
>  	since = NULL;
>  	allow_null = NULL;
> +	is_bitfield = NULL;
>  	for (i = 0; atts[i]; i += 2) {
>  		if (strcmp(atts[i], "name") == 0)
>  			name = atts[i + 1];
> @@ -339,12 +345,18 @@ start_element(void *data, const char *element_name, const char **atts)
>  			value = atts[i + 1];
>  		if (strcmp(atts[i], "interface") == 0)
>  			interface_name = atts[i + 1];
> +		if (strcmp(atts[i], "enum") == 0)
> +			enum_name = atts[i + 1];
> +		if (strcmp(atts[i], "bitfield") == 0)
> +			bitfield_name = atts[i + 1];
>  		if (strcmp(atts[i], "summary") == 0)
>  			summary = atts[i + 1];
>  		if (strcmp(atts[i], "since") == 0)
>  			since = atts[i + 1];
>  		if (strcmp(atts[i], "allow-null") == 0)
>  			allow_null = atts[i + 1];
> +		if (strcmp(atts[i], "is_bitfield") == 0)
> +			is_bitfield = atts[i + 1];
>  	}
>  
>  	ctx->character_data_length = 0;
> @@ -461,9 +473,31 @@ start_element(void *data, const char *element_name, const char **atts)
>  			else
>  				arg->interface_name = NULL;
>  			break;
> +		case UNSIGNED:
> +		case INT:
> +			if (enum_name) {
> +				arg->enum_name = xstrdup(enum_name);
> +				arg->bitfield_name = NULL;
> +			}
> +			else if (bitfield_name) {
> +				arg->bitfield_name = xstrdup(bitfield_name);
> +				arg->enum_name = NULL;
> +			}
> +			else {
> +				arg->bitfield_name = NULL;
> +				arg->enum_name = NULL;
> +			}
> +
> +			if (enum_name && bitfield_name)
> +				fail(&ctx->loc, "bitfield and enum attribute cannot both be set");
> +			break;
>  		default:
>  			if (interface_name != NULL)
>  				fail(&ctx->loc, "interface attribute not allowed for type %s", type);
> +			if (enum_name != NULL)
> +				fail(&ctx->loc, "enum attribute not allowed for type %s", type);
> +			if (bitfield_name != NULL)
> +				fail(&ctx->loc, "bitfield attribute not allowed for type %s", type);

This looks broken.

What if
- int or unsigned has "interface" attribute? It should be an error.
- new_id or object has "enum" or "bitfield" attribute? Should be an
  error.

You probably want your own switch-clause.

>  			break;
>  		}
>  
> @@ -497,6 +531,13 @@ start_element(void *data, const char *element_name, const char **atts)
>  			       &enumeration->link);
>  
>  		ctx->enumeration = enumeration;
> +
> +		if (is_bitfield == NULL || strcmp(is_bitfield, "false") == 0)
> +			enumeration->is_bitfield = 0;
> +		else if (strcmp(is_bitfield, "true") == 0)
> +			enumeration->is_bitfield = 1;
> +		else
> +			fail(&ctx->loc, "invalid value for is_bitfield attribute (%s)", is_bitfield);
>  	} else if (strcmp(element_name, "entry") == 0) {
>  		if (name == NULL)
>  			fail(&ctx->loc, "no entry name given");
> @@ -1246,6 +1287,60 @@ emit_code(struct protocol *protocol)
>  	}
>  }
>  
> +static int
> +enum_exists(struct protocol *protocol, const char *enum_name, int is_bitfield)
> +{
> +	struct interface *i;
> +	struct enumeration *e;
> +	char full_name[64];
> +
> +	wl_list_for_each(i, &protocol->interface_list, link) {
> +		wl_list_for_each(e, &i->enumeration_list, link) {
> +			sprintf(full_name, "%s.%s", i->name, e->name);

Use snprintf to avoid buffer overflows.
Or split enum_name, and match both halves separately.

> +			if (strcmp(full_name, enum_name) == 0 &&
> +			    e->is_bitfield == is_bitfield)
> +				return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void
> +check_message(struct protocol *protocol, struct message *m)
> +{
> +	struct arg *a;
> +
> +	wl_list_for_each(a, &m->arg_list, link)
> +		if ((a->type == UNSIGNED || a->type == INT)) {
> +			if (a->enum_name != NULL &&
> +			    !enum_exists(protocol, a->enum_name, 0)) {
> +				fprintf(stderr, "enum %s does not exist\n",
> +					a->enum_name);
> +				exit(EXIT_FAILURE);
> +			}
> +			else if (a->bitfield_name != NULL &&
> +				 !enum_exists(protocol, a->bitfield_name, 1)) {
> +				fprintf(stderr, "bitfield %s does not exist\n",
> +					a->enum_name);
> +				exit(EXIT_FAILURE);
> +			}
> +		}
> +}
> +
> +static void
> +check_protocol(struct protocol *protocol)
> +{
> +	struct interface *i;
> +	struct message *m;
> +
> +	wl_list_for_each(i, &protocol->interface_list, link) {
> +		wl_list_for_each(m, &i->request_list, link)
> +			check_message(protocol, m);
> +		wl_list_for_each(m, &i->event_list, link)
> +			check_message(protocol, m);
> +	}
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	struct parse_context ctx;
> @@ -1302,6 +1397,10 @@ int main(int argc, char *argv[])
>  
>  	XML_ParserFree(ctx.parser);
>  
> +	/* some checks can only be performed after *
> +	 * the whole protocol has been read in     */
> +	check_protocol(&protocol);
> +
>  	switch (mode) {
>  		case CLIENT_HEADER:
>  			emit_header(&protocol, CLIENT);

Should we also enforce, that only UNSIGNED is valid for bitfields?

So, what do other people think of the idea in this patch?

I want to see some other Wayland developers support this, as I am not
confident with my own judgement here.


Thanks,
pq


More information about the wayland-devel mailing list