<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Sep 22, 2014 at 12:27 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote"><div><div class="h5">On Mon, Sep 22, 2014 at 9:30 AM, Nils Chr. Brause <<a href="mailto:nilschrbrause@gmail.com" target="_blank">nilschrbrause@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On Mon, Sep 22, 2014 at 11:21 AM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: > > On Wed, 3 Sep 2014 19:44:04 +0200 > "Nils Chr. Brause" <<a href="mailto:nilschrbrause@gmail.com" target="_blank">nilschrbrause@gmail.com</a>> 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" <<a href="mailto:nilschrbrause@googlemail.com" target="_blank">nilschrbrause@googlemail.com</a>> > > 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> > > > >  > > @@ -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. </div></div>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. <div><div> > > > 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. </div></div>I will split enum_name then. <div><div> > > > + 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? </div></div>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? </blockquote><div> </div></div></div><div>It's both... More in a couple of lines </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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? </blockquote><div> </div><div>I'm a little unsure. I think trying to completely solve this problem in a way that will truly make strongly typed languages happy is insanity. That said, I'm cautiously ok with defining bitfields and enums as long as we are very careful in scoping what "bitfield" and "enum" mean. A "bitfield" should have only power of two values and the result should always be interpreted as an OR of those values. An enum should have every possible value enumerated. If anyone has a good example of something that validly doesn't fit into either of these, please speak up. </div></div></div></div></blockquote><div> </div><div>xdg_shell.resize_edges. It is both a bitfield (top / left / right / bottom edges are powers of two) and an enum (top right / bottom left corner convenience values, with top / bottom being left out as it is undefined). </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>The example of wl_output.transform is an enum because every possibility is enumerated. From C or a similar language, you can do fun stuff like "if (transform & WL_OUTPUT_TRANSFORM_FLIPPED)" to determine if there is a flip. In a strongly typed language, you can't do this and we shouldn't bend over backwards to make it possible. If we try and come up with some convoluted system that makes this possible with typed languages, we're going to cause far more pain than it's worth. </div><div>One other thing that we need to keep in mind here is the primary target audience of Wayland and its libraries. That audience is compositors and toolkits. Most of those are written in C and C++. What we don't want to do is to do a bunch of things for the sake of 1% of the target audience that makes the rest have to bend over backwards. When I said "cautiously OK", I mean that I don't see that happening yet and I don't see a valid use for an enum that doesn't follow one of those two rules. However, if we have a plausible case where doing so would make everyone's lives easier, I'm going to not be a big fan. </div><div>Please note that I'm not trying to insult Haskel or other functional or strongly typed languages or the people who use them. I'm simply trying to be pragmatic and recognize that people who want to write an app in haskel that manually bangs the Wayland protocol isn't the target audience. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > I want to see some other Wayland developers support this, as I am not > confident with my own judgement here. > > > Thanks, > pq Cheers, Nils <div><div>_______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org" target="_blank">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </div></div></blockquote></div> </div></div> _______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote></div> -- Jasper </div></div>