On Mon, Sep 22, 2014 at 11:21 AM, Pekka Paalanen <ppaala...@gmail.com> wrote: > > On Wed, 3 Sep 2014 19:44:04 +0200 > "Nils Chr. Brause" <nilschrbra...@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" <nilschrbra...@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.
Wouldn't a separate switch-clause require a new type? I thought, this is not what we want, since this would break other parsers. I would probably just check for additional unwanted combinations. > > > 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. I will split enum_name then. > > > + 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? I would certainly like to do this, but wl_output.transform is always passed as a signed integer and I'm not sure if this is a bitfield or not. I assumed it is, because you can reconstruct "270", "flipped_90", "flipped_180" and "flipped_270" out of "90", "180" and "flipped". Or is this just a coincidence? Also, is there a reason why some enumerations are passed as a signed integer (wl_output.subpixel) and others aren't? > > 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 Cheers, Nils _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel