[v3] protocol: add support for cross-interface enum attributes

Submitted by Auke Booij on Dec. 5, 2015, 12:39 p.m.

Details

Message ID 1449319152-11539-1-git-send-email-auke@tulcod.com
State Accepted
Delegated to: Pekka Paalanen
Headers show
Series "protocol: add support for cross-interface enum attributes" ( rev: 2 ) in Wayland

Not browsing as part of any series.

Commit Message

Auke Booij Dec. 5, 2015, 12:39 p.m.
The enum attribute, for which scanner support was introduced in
1771299, can be used to link message arguments to <enum>s. However,
some arguments refer to <enum>s in a different <interface>.

This adds scanner support for referring to an <enum> in a different
<interface> using dot notation. It also sets the attributes in this
style in the wayland XML protocol (wl_shm_pool::create_buffer::format
to wl_shm::format, and wl_surface::set_buffer_transform::transform to
wl_output::transform), and updates the documentation XSL so that this
new style is supported.

Changes since v2:
 - add object:: prefix for all enumerations in the documentation
 - fix whitespace in scanner.c
 - minor code fixup to return early and avoid casts in scanner.c

Changes since v1:
 - several implementation bugs fixed

Signed-off-by: Auke Booij <auke@tulcod.com>
---
 doc/publican/protocol-to-docbook.xsl | 19 +++++++++--
 protocol/wayland.xml                 |  4 +--
 src/scanner.c                        | 63 +++++++++++++++++++++++++++---------
 3 files changed, 65 insertions(+), 21 deletions(-)

Patch hide | download patch | download mbox

diff --git a/doc/publican/protocol-to-docbook.xsl b/doc/publican/protocol-to-docbook.xsl
index fad207a..7e892c3 100644
--- a/doc/publican/protocol-to-docbook.xsl
+++ b/doc/publican/protocol-to-docbook.xsl
@@ -103,9 +103,22 @@ 
     <listitem>
         <simpara>
           <xsl:if test="@enum">
-            <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
-              <xsl:value-of select="@enum"/>
-            </link>
+            <xsl:choose>
+              <xsl:when test="contains(@enum, '.')">
+                <link linkend="protocol-spec-{substring-before(@enum, '.')}-enum-{substring-after(@enum, '.')}">
+                  <xsl:value-of select="substring-before(@enum, '.')"/>
+                  <xsl:text>::</xsl:text>
+                  <xsl:value-of select="substring-after(@enum, '.')"/>
+                </link>
+              </xsl:when>
+              <xsl:otherwise>
+                <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
+                  <xsl:value-of select="../../@name"/>
+                  <xsl:text>::</xsl:text>
+                  <xsl:value-of select="@enum"/>
+                </link>
+              </xsl:otherwise>
+            </xsl:choose>
 	    <xsl:text> </xsl:text>
           </xsl:if>
           <xsl:value-of select="@type"/>
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index f9e6d76..0873553 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">
@@ -1292,7 +1292,7 @@ 
 	wl_output.transform enum the invalid_transform protocol error
 	is raised.
       </description>
-      <arg name="transform" type="int"/>
+      <arg name="transform" type="int" enum="wl_output.transform"/>
     </request>
 
     <!-- Version 3 additions -->
diff --git a/src/scanner.c b/src/scanner.c
index 406519f..85aeea7 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -747,8 +747,8 @@  start_element(void *data, const char *element_name, const char **atts)
 			enumeration->bitfield = true;
 		else
 			fail(&ctx->loc,
-                             "invalid value (%s) for bitfield attribute (only true/false are accepted)",
-                             bitfield);
+			     "invalid value (%s) for bitfield attribute (only true/false are accepted)",
+			     bitfield);
 
 		wl_list_insert(ctx->interface->enumeration_list.prev,
 			       &enumeration->link);
@@ -785,32 +785,62 @@  start_element(void *data, const char *element_name, const char **atts)
 	}
 }
 
+static struct enumeration *
+find_enumeration(struct protocol *protocol, struct interface *interface, char *enum_attribute)
+{
+	struct interface *i;
+	struct enumeration *e;
+	char *enum_name;
+	uint idx = 0, j;
+
+	for (j = 0; j + 1 < strlen(enum_attribute); j++) {
+		if (enum_attribute[j] == '.') {
+			idx = j;
+		}
+	}
+
+	if (idx > 0) {
+		enum_name = enum_attribute + idx + 1;
+
+		wl_list_for_each(i, &protocol->interface_list, link)
+			if (strncmp(i->name, enum_attribute, idx) == 0)
+				wl_list_for_each(e, &i->enumeration_list, link)
+					if(strcmp(e->name, enum_name) == 0)
+						return e;
+	} else if (interface) {
+		enum_name = enum_attribute;
+
+		wl_list_for_each(e, &interface->enumeration_list, link)
+			if(strcmp(e->name, enum_name) == 0)
+				return e;
+	}
+
+	return NULL;
+}
+
 static void
-verify_arguments(struct parse_context *ctx, struct wl_list *messages, struct wl_list *enumerations)
+verify_arguments(struct parse_context *ctx, struct interface *interface, 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;
+			struct enumeration *e;
 
 			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)
+			e = find_enumeration(ctx->protocol, interface, a->enumeration_name);
+
+			if (e == NULL)
 				fail(&ctx->loc,
 				     "could not find enumeration %s",
 				     a->enumeration_name);
 
 			switch (a->type) {
 			case INT:
-				if (f->bitfield)
+				if (e->bitfield)
 					fail(&ctx->loc,
 					     "bitfield-style enum must only be referenced by uint");
 				break;
@@ -848,12 +878,13 @@  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);
+	} else if (strcmp(name, "protocol") == 0) {
+		struct interface *i;
 
+		wl_list_for_each(i, &ctx->protocol->interface_list, link) {
+			verify_arguments(ctx, i, &i->request_list, &i->enumeration_list);
+			verify_arguments(ctx, i, &i->event_list, &i->enumeration_list);
+		}
 	}
 }
 

Comments

Awesome! I hope that gets pushed soon. :)

Reviewed-by: Nils Christopher Brause <nilschrbrause@googlemail.com>


On Sat, Dec 5, 2015 at 1:39 PM, Auke Booij <auke@tulcod.com> wrote:
> The enum attribute, for which scanner support was introduced in
> 1771299, can be used to link message arguments to <enum>s. However,
> some arguments refer to <enum>s in a different <interface>.
>
> This adds scanner support for referring to an <enum> in a different
> <interface> using dot notation. It also sets the attributes in this
> style in the wayland XML protocol (wl_shm_pool::create_buffer::format
> to wl_shm::format, and wl_surface::set_buffer_transform::transform to
> wl_output::transform), and updates the documentation XSL so that this
> new style is supported.
>
> Changes since v2:
>  - add object:: prefix for all enumerations in the documentation
>  - fix whitespace in scanner.c
>  - minor code fixup to return early and avoid casts in scanner.c
>
> Changes since v1:
>  - several implementation bugs fixed
>
> Signed-off-by: Auke Booij <auke@tulcod.com>
> ---
>  doc/publican/protocol-to-docbook.xsl | 19 +++++++++--
>  protocol/wayland.xml                 |  4 +--
>  src/scanner.c                        | 63 +++++++++++++++++++++++++++---------
>  3 files changed, 65 insertions(+), 21 deletions(-)
>
> diff --git a/doc/publican/protocol-to-docbook.xsl b/doc/publican/protocol-to-docbook.xsl
> index fad207a..7e892c3 100644
> --- a/doc/publican/protocol-to-docbook.xsl
> +++ b/doc/publican/protocol-to-docbook.xsl
> @@ -103,9 +103,22 @@
>      <listitem>
>          <simpara>
>            <xsl:if test="@enum">
> -            <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
> -              <xsl:value-of select="@enum"/>
> -            </link>
> +            <xsl:choose>
> +              <xsl:when test="contains(@enum, '.')">
> +                <link linkend="protocol-spec-{substring-before(@enum, '.')}-enum-{substring-after(@enum, '.')}">
> +                  <xsl:value-of select="substring-before(@enum, '.')"/>
> +                  <xsl:text>::</xsl:text>
> +                  <xsl:value-of select="substring-after(@enum, '.')"/>
> +                </link>
> +              </xsl:when>
> +              <xsl:otherwise>
> +                <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
> +                  <xsl:value-of select="../../@name"/>
> +                  <xsl:text>::</xsl:text>
> +                  <xsl:value-of select="@enum"/>
> +                </link>
> +              </xsl:otherwise>
> +            </xsl:choose>
>             <xsl:text> </xsl:text>
>            </xsl:if>
>            <xsl:value-of select="@type"/>
> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index f9e6d76..0873553 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">
> @@ -1292,7 +1292,7 @@
>         wl_output.transform enum the invalid_transform protocol error
>         is raised.
>        </description>
> -      <arg name="transform" type="int"/>
> +      <arg name="transform" type="int" enum="wl_output.transform"/>
>      </request>
>
>      <!-- Version 3 additions -->
> diff --git a/src/scanner.c b/src/scanner.c
> index 406519f..85aeea7 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -747,8 +747,8 @@ start_element(void *data, const char *element_name, const char **atts)
>                         enumeration->bitfield = true;
>                 else
>                         fail(&ctx->loc,
> -                             "invalid value (%s) for bitfield attribute (only true/false are accepted)",
> -                             bitfield);
> +                            "invalid value (%s) for bitfield attribute (only true/false are accepted)",
> +                            bitfield);
>
>                 wl_list_insert(ctx->interface->enumeration_list.prev,
>                                &enumeration->link);
> @@ -785,32 +785,62 @@ start_element(void *data, const char *element_name, const char **atts)
>         }
>  }
>
> +static struct enumeration *
> +find_enumeration(struct protocol *protocol, struct interface *interface, char *enum_attribute)
> +{
> +       struct interface *i;
> +       struct enumeration *e;
> +       char *enum_name;
> +       uint idx = 0, j;
> +
> +       for (j = 0; j + 1 < strlen(enum_attribute); j++) {
> +               if (enum_attribute[j] == '.') {
> +                       idx = j;
> +               }
> +       }
> +
> +       if (idx > 0) {
> +               enum_name = enum_attribute + idx + 1;
> +
> +               wl_list_for_each(i, &protocol->interface_list, link)
> +                       if (strncmp(i->name, enum_attribute, idx) == 0)
> +                               wl_list_for_each(e, &i->enumeration_list, link)
> +                                       if(strcmp(e->name, enum_name) == 0)
> +                                               return e;
> +       } else if (interface) {
> +               enum_name = enum_attribute;
> +
> +               wl_list_for_each(e, &interface->enumeration_list, link)
> +                       if(strcmp(e->name, enum_name) == 0)
> +                               return e;
> +       }
> +
> +       return NULL;
> +}
> +
>  static void
> -verify_arguments(struct parse_context *ctx, struct wl_list *messages, struct wl_list *enumerations)
> +verify_arguments(struct parse_context *ctx, struct interface *interface, 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;
> +                       struct enumeration *e;
>
>                         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)
> +                       e = find_enumeration(ctx->protocol, interface, a->enumeration_name);
> +
> +                       if (e == NULL)
>                                 fail(&ctx->loc,
>                                      "could not find enumeration %s",
>                                      a->enumeration_name);
>
>                         switch (a->type) {
>                         case INT:
> -                               if (f->bitfield)
> +                               if (e->bitfield)
>                                         fail(&ctx->loc,
>                                              "bitfield-style enum must only be referenced by uint");
>                                 break;
> @@ -848,12 +878,13 @@ 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);
> +       } else if (strcmp(name, "protocol") == 0) {
> +               struct interface *i;
>
> +               wl_list_for_each(i, &ctx->protocol->interface_list, link) {
> +                       verify_arguments(ctx, i, &i->request_list, &i->enumeration_list);
> +                       verify_arguments(ctx, i, &i->event_list, &i->enumeration_list);
> +               }
>         }
>  }
>
> --
> 2.6.2
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Bill, any chance you could review this? Or would you prefer it if this
were based on your patch (which I do still support)?

On 5 December 2015 at 13:39, Auke Booij <auke@tulcod.com> wrote:
> The enum attribute, for which scanner support was introduced in
> 1771299, can be used to link message arguments to <enum>s. However,
> some arguments refer to <enum>s in a different <interface>.
>
> This adds scanner support for referring to an <enum> in a different
> <interface> using dot notation. It also sets the attributes in this
> style in the wayland XML protocol (wl_shm_pool::create_buffer::format
> to wl_shm::format, and wl_surface::set_buffer_transform::transform to
> wl_output::transform), and updates the documentation XSL so that this
> new style is supported.
>
> Changes since v2:
>  - add object:: prefix for all enumerations in the documentation
>  - fix whitespace in scanner.c
>  - minor code fixup to return early and avoid casts in scanner.c
>
> Changes since v1:
>  - several implementation bugs fixed
>
> Signed-off-by: Auke Booij <auke@tulcod.com>
> ---
>  doc/publican/protocol-to-docbook.xsl | 19 +++++++++--
>  protocol/wayland.xml                 |  4 +--
>  src/scanner.c                        | 63 +++++++++++++++++++++++++++---------
>  3 files changed, 65 insertions(+), 21 deletions(-)
>
> diff --git a/doc/publican/protocol-to-docbook.xsl b/doc/publican/protocol-to-docbook.xsl
> index fad207a..7e892c3 100644
> --- a/doc/publican/protocol-to-docbook.xsl
> +++ b/doc/publican/protocol-to-docbook.xsl
> @@ -103,9 +103,22 @@
>      <listitem>
>          <simpara>
>            <xsl:if test="@enum">
> -            <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
> -              <xsl:value-of select="@enum"/>
> -            </link>
> +            <xsl:choose>
> +              <xsl:when test="contains(@enum, '.')">
> +                <link linkend="protocol-spec-{substring-before(@enum, '.')}-enum-{substring-after(@enum, '.')}">
> +                  <xsl:value-of select="substring-before(@enum, '.')"/>
> +                  <xsl:text>::</xsl:text>
> +                  <xsl:value-of select="substring-after(@enum, '.')"/>
> +                </link>
> +              </xsl:when>
> +              <xsl:otherwise>
> +                <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
> +                  <xsl:value-of select="../../@name"/>
> +                  <xsl:text>::</xsl:text>
> +                  <xsl:value-of select="@enum"/>
> +                </link>
> +              </xsl:otherwise>
> +            </xsl:choose>
>             <xsl:text> </xsl:text>
>            </xsl:if>
>            <xsl:value-of select="@type"/>
> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index f9e6d76..0873553 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">
> @@ -1292,7 +1292,7 @@
>         wl_output.transform enum the invalid_transform protocol error
>         is raised.
>        </description>
> -      <arg name="transform" type="int"/>
> +      <arg name="transform" type="int" enum="wl_output.transform"/>
>      </request>
>
>      <!-- Version 3 additions -->
> diff --git a/src/scanner.c b/src/scanner.c
> index 406519f..85aeea7 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -747,8 +747,8 @@ start_element(void *data, const char *element_name, const char **atts)
>                         enumeration->bitfield = true;
>                 else
>                         fail(&ctx->loc,
> -                             "invalid value (%s) for bitfield attribute (only true/false are accepted)",
> -                             bitfield);
> +                            "invalid value (%s) for bitfield attribute (only true/false are accepted)",
> +                            bitfield);
>
>                 wl_list_insert(ctx->interface->enumeration_list.prev,
>                                &enumeration->link);
> @@ -785,32 +785,62 @@ start_element(void *data, const char *element_name, const char **atts)
>         }
>  }
>
> +static struct enumeration *
> +find_enumeration(struct protocol *protocol, struct interface *interface, char *enum_attribute)
> +{
> +       struct interface *i;
> +       struct enumeration *e;
> +       char *enum_name;
> +       uint idx = 0, j;
> +
> +       for (j = 0; j + 1 < strlen(enum_attribute); j++) {
> +               if (enum_attribute[j] == '.') {
> +                       idx = j;
> +               }
> +       }
> +
> +       if (idx > 0) {
> +               enum_name = enum_attribute + idx + 1;
> +
> +               wl_list_for_each(i, &protocol->interface_list, link)
> +                       if (strncmp(i->name, enum_attribute, idx) == 0)
> +                               wl_list_for_each(e, &i->enumeration_list, link)
> +                                       if(strcmp(e->name, enum_name) == 0)
> +                                               return e;
> +       } else if (interface) {
> +               enum_name = enum_attribute;
> +
> +               wl_list_for_each(e, &interface->enumeration_list, link)
> +                       if(strcmp(e->name, enum_name) == 0)
> +                               return e;
> +       }
> +
> +       return NULL;
> +}
> +
>  static void
> -verify_arguments(struct parse_context *ctx, struct wl_list *messages, struct wl_list *enumerations)
> +verify_arguments(struct parse_context *ctx, struct interface *interface, 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;
> +                       struct enumeration *e;
>
>                         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)
> +                       e = find_enumeration(ctx->protocol, interface, a->enumeration_name);
> +
> +                       if (e == NULL)
>                                 fail(&ctx->loc,
>                                      "could not find enumeration %s",
>                                      a->enumeration_name);
>
>                         switch (a->type) {
>                         case INT:
> -                               if (f->bitfield)
> +                               if (e->bitfield)
>                                         fail(&ctx->loc,
>                                              "bitfield-style enum must only be referenced by uint");
>                                 break;
> @@ -848,12 +878,13 @@ 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);
> +       } else if (strcmp(name, "protocol") == 0) {
> +               struct interface *i;
>
> +               wl_list_for_each(i, &ctx->protocol->interface_list, link) {
> +                       verify_arguments(ctx, i, &i->request_list, &i->enumeration_list);
> +                       verify_arguments(ctx, i, &i->event_list, &i->enumeration_list);
> +               }
>         }
>  }
>
> --
> 2.6.2
>
I believe this is correct and worth using.

The only difference with mine is that the protocol-to-docbook.xsl puts the
enum arg handling into it's own top-level clause, rather than as an if
statement. This I think makes it more consistent with the other argument
special-cases (one for object ids, and another for new ids). I'm not sure
if this is really important, and also I think I could submit it as an extra
patch after this.


On Mon, Dec 14, 2015 at 4:48 AM, Auke Booij <auke@tulcod.com> wrote:

> Bill, any chance you could review this? Or would you prefer it if this
> were based on your patch (which I do still support)?
>
> On 5 December 2015 at 13:39, Auke Booij <auke@tulcod.com> wrote:
> > The enum attribute, for which scanner support was introduced in
> > 1771299, can be used to link message arguments to <enum>s. However,
> > some arguments refer to <enum>s in a different <interface>.
> >
> > This adds scanner support for referring to an <enum> in a different
> > <interface> using dot notation. It also sets the attributes in this
> > style in the wayland XML protocol (wl_shm_pool::create_buffer::format
> > to wl_shm::format, and wl_surface::set_buffer_transform::transform to
> > wl_output::transform), and updates the documentation XSL so that this
> > new style is supported.
> >
> > Changes since v2:
> >  - add object:: prefix for all enumerations in the documentation
> >  - fix whitespace in scanner.c
> >  - minor code fixup to return early and avoid casts in scanner.c
> >
> > Changes since v1:
> >  - several implementation bugs fixed
> >
> > Signed-off-by: Auke Booij <auke@tulcod.com>
> > ---
> >  doc/publican/protocol-to-docbook.xsl | 19 +++++++++--
> >  protocol/wayland.xml                 |  4 +--
> >  src/scanner.c                        | 63
> +++++++++++++++++++++++++++---------
> >  3 files changed, 65 insertions(+), 21 deletions(-)
> >
> > diff --git a/doc/publican/protocol-to-docbook.xsl
> b/doc/publican/protocol-to-docbook.xsl
> > index fad207a..7e892c3 100644
> > --- a/doc/publican/protocol-to-docbook.xsl
> > +++ b/doc/publican/protocol-to-docbook.xsl
> > @@ -103,9 +103,22 @@
> >      <listitem>
> >          <simpara>
> >            <xsl:if test="@enum">
> > -            <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
> > -              <xsl:value-of select="@enum"/>
> > -            </link>
> > +            <xsl:choose>
> > +              <xsl:when test="contains(@enum, '.')">
> > +                <link linkend="protocol-spec-{substring-before(@enum,
> '.')}-enum-{substring-after(@enum, '.')}">
> > +                  <xsl:value-of select="substring-before(@enum, '.')"/>
> > +                  <xsl:text>::</xsl:text>
> > +                  <xsl:value-of select="substring-after(@enum, '.')"/>
> > +                </link>
> > +              </xsl:when>
> > +              <xsl:otherwise>
> > +                <link linkend="protocol-spec-{../../@name}-enum-{@enum
> }">
> > +                  <xsl:value-of select="../../@name"/>
> > +                  <xsl:text>::</xsl:text>
> > +                  <xsl:value-of select="@enum"/>
> > +                </link>
> > +              </xsl:otherwise>
> > +            </xsl:choose>
> >             <xsl:text> </xsl:text>
> >            </xsl:if>
> >            <xsl:value-of select="@type"/>
> > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> > index f9e6d76..0873553 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">
> > @@ -1292,7 +1292,7 @@
> >         wl_output.transform enum the invalid_transform protocol error
> >         is raised.
> >        </description>
> > -      <arg name="transform" type="int"/>
> > +      <arg name="transform" type="int" enum="wl_output.transform"/>
> >      </request>
> >
> >      <!-- Version 3 additions -->
> > diff --git a/src/scanner.c b/src/scanner.c
> > index 406519f..85aeea7 100644
> > --- a/src/scanner.c
> > +++ b/src/scanner.c
> > @@ -747,8 +747,8 @@ start_element(void *data, const char *element_name,
> const char **atts)
> >                         enumeration->bitfield = true;
> >                 else
> >                         fail(&ctx->loc,
> > -                             "invalid value (%s) for bitfield attribute
> (only true/false are accepted)",
> > -                             bitfield);
> > +                            "invalid value (%s) for bitfield attribute
> (only true/false are accepted)",
> > +                            bitfield);
> >
> >                 wl_list_insert(ctx->interface->enumeration_list.prev,
> >                                &enumeration->link);
> > @@ -785,32 +785,62 @@ start_element(void *data, const char
> *element_name, const char **atts)
> >         }
> >  }
> >
> > +static struct enumeration *
> > +find_enumeration(struct protocol *protocol, struct interface
> *interface, char *enum_attribute)
> > +{
> > +       struct interface *i;
> > +       struct enumeration *e;
> > +       char *enum_name;
> > +       uint idx = 0, j;
> > +
> > +       for (j = 0; j + 1 < strlen(enum_attribute); j++) {
> > +               if (enum_attribute[j] == '.') {
> > +                       idx = j;
> > +               }
> > +       }
> > +
> > +       if (idx > 0) {
> > +               enum_name = enum_attribute + idx + 1;
> > +
> > +               wl_list_for_each(i, &protocol->interface_list, link)
> > +                       if (strncmp(i->name, enum_attribute, idx) == 0)
> > +                               wl_list_for_each(e,
> &i->enumeration_list, link)
> > +                                       if(strcmp(e->name, enum_name) ==
> 0)
> > +                                               return e;
> > +       } else if (interface) {
> > +               enum_name = enum_attribute;
> > +
> > +               wl_list_for_each(e, &interface->enumeration_list, link)
> > +                       if(strcmp(e->name, enum_name) == 0)
> > +                               return e;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> >  static void
> > -verify_arguments(struct parse_context *ctx, struct wl_list *messages,
> struct wl_list *enumerations)
> > +verify_arguments(struct parse_context *ctx, struct interface
> *interface, 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;
> > +                       struct enumeration *e;
> >
> >                         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)
> > +                       e = find_enumeration(ctx->protocol, interface,
> a->enumeration_name);
> > +
> > +                       if (e == NULL)
> >                                 fail(&ctx->loc,
> >                                      "could not find enumeration %s",
> >                                      a->enumeration_name);
> >
> >                         switch (a->type) {
> >                         case INT:
> > -                               if (f->bitfield)
> > +                               if (e->bitfield)
> >                                         fail(&ctx->loc,
> >                                              "bitfield-style enum must
> only be referenced by uint");
> >                                 break;
> > @@ -848,12 +878,13 @@ 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);
> > +       } else if (strcmp(name, "protocol") == 0) {
> > +               struct interface *i;
> >
> > +               wl_list_for_each(i, &ctx->protocol->interface_list,
> link) {
> > +                       verify_arguments(ctx, i, &i->request_list,
> &i->enumeration_list);
> > +                       verify_arguments(ctx, i, &i->event_list,
> &i->enumeration_list);
> > +               }
> >         }
> >  }
> >
> > --
> > 2.6.2
> >
>
On Sat,  5 Dec 2015 12:39:12 +0000
Auke Booij <auke@tulcod.com> wrote:

> The enum attribute, for which scanner support was introduced in
> 1771299, can be used to link message arguments to <enum>s. However,
> some arguments refer to <enum>s in a different <interface>.
> 
> This adds scanner support for referring to an <enum> in a different
> <interface> using dot notation. It also sets the attributes in this
> style in the wayland XML protocol (wl_shm_pool::create_buffer::format
> to wl_shm::format, and wl_surface::set_buffer_transform::transform to
> wl_output::transform), and updates the documentation XSL so that this
> new style is supported.
> 
> Changes since v2:
>  - add object:: prefix for all enumerations in the documentation
>  - fix whitespace in scanner.c
>  - minor code fixup to return early and avoid casts in scanner.c
> 
> Changes since v1:
>  - several implementation bugs fixed
> 
> Signed-off-by: Auke Booij <auke@tulcod.com>
> ---
>  doc/publican/protocol-to-docbook.xsl | 19 +++++++++--
>  protocol/wayland.xml                 |  4 +--
>  src/scanner.c                        | 63 +++++++++++++++++++++++++++---------
>  3 files changed, 65 insertions(+), 21 deletions(-)
> 
> diff --git a/doc/publican/protocol-to-docbook.xsl b/doc/publican/protocol-to-docbook.xsl
> index fad207a..7e892c3 100644
> --- a/doc/publican/protocol-to-docbook.xsl
> +++ b/doc/publican/protocol-to-docbook.xsl
> @@ -103,9 +103,22 @@
>      <listitem>
>          <simpara>
>            <xsl:if test="@enum">
> -            <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
> -              <xsl:value-of select="@enum"/>
> -            </link>
> +            <xsl:choose>
> +              <xsl:when test="contains(@enum, '.')">
> +                <link linkend="protocol-spec-{substring-before(@enum, '.')}-enum-{substring-after(@enum, '.')}">
> +                  <xsl:value-of select="substring-before(@enum, '.')"/>
> +                  <xsl:text>::</xsl:text>
> +                  <xsl:value-of select="substring-after(@enum, '.')"/>
> +                </link>
> +              </xsl:when>
> +              <xsl:otherwise>
> +                <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
> +                  <xsl:value-of select="../../@name"/>
> +                  <xsl:text>::</xsl:text>
> +                  <xsl:value-of select="@enum"/>
> +                </link>
> +              </xsl:otherwise>
> +            </xsl:choose>
>  	    <xsl:text> </xsl:text>
>            </xsl:if>
>            <xsl:value-of select="@type"/>

Hi,

thanks for this.

This XSL snippet does not work when the enum is defined in a different
XML file, does it? Just making sure I know what the expected behaviour
is.

> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index f9e6d76..0873553 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">
> @@ -1292,7 +1292,7 @@
>  	wl_output.transform enum the invalid_transform protocol error
>  	is raised.
>        </description>
> -      <arg name="transform" type="int"/>
> +      <arg name="transform" type="int" enum="wl_output.transform"/>
>      </request>
>  

These look good.

>      <!-- Version 3 additions -->
> diff --git a/src/scanner.c b/src/scanner.c
> index 406519f..85aeea7 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -747,8 +747,8 @@ start_element(void *data, const char *element_name, const char **atts)
>  			enumeration->bitfield = true;
>  		else
>  			fail(&ctx->loc,
> -                             "invalid value (%s) for bitfield attribute (only true/false are accepted)",
> -                             bitfield);
> +			     "invalid value (%s) for bitfield attribute (only true/false are accepted)",
> +			     bitfield);

This looks like a spurious change for this patch, but I let it pass.

>  
>  		wl_list_insert(ctx->interface->enumeration_list.prev,
>  			       &enumeration->link);
> @@ -785,32 +785,62 @@ start_element(void *data, const char *element_name, const char **atts)
>  	}
>  }
>  
> +static struct enumeration *
> +find_enumeration(struct protocol *protocol, struct interface *interface, char *enum_attribute)

Wrap long lines.

> +{
> +	struct interface *i;
> +	struct enumeration *e;
> +	char *enum_name;
> +	uint idx = 0, j;
> +
> +	for (j = 0; j + 1 < strlen(enum_attribute); j++) {
> +		if (enum_attribute[j] == '.') {
> +			idx = j;
> +		}
> +	}

Please use strchr() instead.

> +
> +	if (idx > 0) {
> +		enum_name = enum_attribute + idx + 1;
> +
> +		wl_list_for_each(i, &protocol->interface_list, link)
> +			if (strncmp(i->name, enum_attribute, idx) == 0)
> +				wl_list_for_each(e, &i->enumeration_list, link)
> +					if(strcmp(e->name, enum_name) == 0)
> +						return e;
> +	} else if (interface) {
> +		enum_name = enum_attribute;
> +
> +		wl_list_for_each(e, &interface->enumeration_list, link)
> +			if(strcmp(e->name, enum_name) == 0)
> +				return e;

Missing a space after keyword in both copies.

I'd recommend factoring that to a new function
static struct enumeration *
interface_find_enumeration(const struct *interface, const char *enum_name)
or just reordering to avoid the duplicate code.

> +	}
> +
> +	return NULL;
> +}
> +
>  static void
> -verify_arguments(struct parse_context *ctx, struct wl_list *messages, struct wl_list *enumerations)
> +verify_arguments(struct parse_context *ctx, struct interface *interface, struct wl_list *messages, struct wl_list *enumerations)

Wrap long line.

>  {
>  	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;
> +			struct enumeration *e;
>  
>  			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)
> +			e = find_enumeration(ctx->protocol, interface, a->enumeration_name);
> +
> +			if (e == NULL)
>  				fail(&ctx->loc,
>  				     "could not find enumeration %s",
>  				     a->enumeration_name);

Ok, so we do not support enums in different XML files. Those will have
to go without a reference for now.

>  
>  			switch (a->type) {
>  			case INT:
> -				if (f->bitfield)
> +				if (e->bitfield)
>  					fail(&ctx->loc,
>  					     "bitfield-style enum must only be referenced by uint");
>  				break;
> @@ -848,12 +878,13 @@ 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);
> +	} else if (strcmp(name, "protocol") == 0) {
> +		struct interface *i;
>  
> +		wl_list_for_each(i, &ctx->protocol->interface_list, link) {
> +			verify_arguments(ctx, i, &i->request_list, &i->enumeration_list);
> +			verify_arguments(ctx, i, &i->event_list, &i->enumeration_list);
> +		}
>  	}
>  }
>  

Ok, there would be quite some things to polish, but since this patch
has waited for so long so I just rebased the xsl hunk myself, made some
whitespace fixes and pushed it:
   e21aeb5..7ccf35d  master -> master

Please have a look at the merged patch in case I missed something.

If you want to do more clean-ups, those can land after the alpha release.


Thanks,
pq
On 3 May 2016 at 13:04, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Sat,  5 Dec 2015 12:39:12 +0000
> Auke Booij <auke@tulcod.com> wrote:
>
>> The enum attribute, for which scanner support was introduced in
>> 1771299, can be used to link message arguments to <enum>s. However,
>> some arguments refer to <enum>s in a different <interface>.
>>
>> This adds scanner support for referring to an <enum> in a different
>> <interface> using dot notation. It also sets the attributes in this
>> style in the wayland XML protocol (wl_shm_pool::create_buffer::format
>> to wl_shm::format, and wl_surface::set_buffer_transform::transform to
>> wl_output::transform), and updates the documentation XSL so that this
>> new style is supported.
>>
>> Changes since v2:
>>  - add object:: prefix for all enumerations in the documentation
>>  - fix whitespace in scanner.c
>>  - minor code fixup to return early and avoid casts in scanner.c
>>
>> Changes since v1:
>>  - several implementation bugs fixed
>>
>> Signed-off-by: Auke Booij <auke@tulcod.com>
>> ---
>>  doc/publican/protocol-to-docbook.xsl | 19 +++++++++--
>>  protocol/wayland.xml                 |  4 +--
>>  src/scanner.c                        | 63 +++++++++++++++++++++++++++---------
>>  3 files changed, 65 insertions(+), 21 deletions(-)
>>
>> diff --git a/doc/publican/protocol-to-docbook.xsl b/doc/publican/protocol-to-docbook.xsl
>> index fad207a..7e892c3 100644
>> --- a/doc/publican/protocol-to-docbook.xsl
>> +++ b/doc/publican/protocol-to-docbook.xsl
>> @@ -103,9 +103,22 @@
>>      <listitem>
>>          <simpara>
>>            <xsl:if test="@enum">
>> -            <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
>> -              <xsl:value-of select="@enum"/>
>> -            </link>
>> +            <xsl:choose>
>> +              <xsl:when test="contains(@enum, '.')">
>> +                <link linkend="protocol-spec-{substring-before(@enum, '.')}-enum-{substring-after(@enum, '.')}">
>> +                  <xsl:value-of select="substring-before(@enum, '.')"/>
>> +                  <xsl:text>::</xsl:text>
>> +                  <xsl:value-of select="substring-after(@enum, '.')"/>
>> +                </link>
>> +              </xsl:when>
>> +              <xsl:otherwise>
>> +                <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
>> +                  <xsl:value-of select="../../@name"/>
>> +                  <xsl:text>::</xsl:text>
>> +                  <xsl:value-of select="@enum"/>
>> +                </link>
>> +              </xsl:otherwise>
>> +            </xsl:choose>
>>           <xsl:text> </xsl:text>
>>            </xsl:if>
>>            <xsl:value-of select="@type"/>
>
> Hi,
>
> thanks for this.
>
> This XSL snippet does not work when the enum is defined in a different
> XML file, does it? Just making sure I know what the expected behaviour
> is.

Correct; I don't think this is worth implementing at this stage.

> Ok, there would be quite some things to polish, but since this patch
> has waited for so long so I just rebased the xsl hunk myself, made some
> whitespace fixes and pushed it:
>    e21aeb5..7ccf35d  master -> master
>
> Please have a look at the merged patch in case I missed something.
>
> If you want to do more clean-ups, those can land after the alpha release.
>
>
> Thanks,
> pq

The patch you pushed looks correct; thanks!
Very nice to see that finally landing! :)
Since this patch has been created by Auke Booij back in 2015 (thank
you very much!), there have been some protocol additions in the DnD
area, which also need proper enum attributes. From the wayland.xml of
the C++ Bindings[1]:

wayland.xml:543:      <arg name="dnd_actions" type="uint"
enum="wl_data_device_manager.dnd_action"/>
wayland.xml:544:      <arg name="preferred_action" type="uint"
enum="wl_data_device_manager.dnd_action"/>
wayland.xml:553:      <arg name="source_actions" type="uint"
enum="wl_data_device_manager.dnd_action"/>
wayland.xml:594:      <arg name="dnd_action" type="uint"
enum="wl_data_device_manager.dnd_action"/>
wayland.xml:693:      <arg name="dnd_actions" type="uint"
enum="wl_data_device_manager.dnd_action"/>
wayland.xml:749:      <arg name="dnd_action" type="uint"
enum="wl_data_device_manager.dnd_action"/>

Please add those, too.

Cheers,
Nils

[1] https://github.com/NilsBrause/waylandpp


On Tue, May 3, 2016 at 9:57 PM, Auke Booij <auke@tulcod.com> wrote:
> On 3 May 2016 at 13:04, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>> On Sat,  5 Dec 2015 12:39:12 +0000
>> Auke Booij <auke@tulcod.com> wrote:
>>
>>> The enum attribute, for which scanner support was introduced in
>>> 1771299, can be used to link message arguments to <enum>s. However,
>>> some arguments refer to <enum>s in a different <interface>.
>>>
>>> This adds scanner support for referring to an <enum> in a different
>>> <interface> using dot notation. It also sets the attributes in this
>>> style in the wayland XML protocol (wl_shm_pool::create_buffer::format
>>> to wl_shm::format, and wl_surface::set_buffer_transform::transform to
>>> wl_output::transform), and updates the documentation XSL so that this
>>> new style is supported.
>>>
>>> Changes since v2:
>>>  - add object:: prefix for all enumerations in the documentation
>>>  - fix whitespace in scanner.c
>>>  - minor code fixup to return early and avoid casts in scanner.c
>>>
>>> Changes since v1:
>>>  - several implementation bugs fixed
>>>
>>> Signed-off-by: Auke Booij <auke@tulcod.com>
>>> ---
>>>  doc/publican/protocol-to-docbook.xsl | 19 +++++++++--
>>>  protocol/wayland.xml                 |  4 +--
>>>  src/scanner.c                        | 63 +++++++++++++++++++++++++++---------
>>>  3 files changed, 65 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/doc/publican/protocol-to-docbook.xsl b/doc/publican/protocol-to-docbook.xsl
>>> index fad207a..7e892c3 100644
>>> --- a/doc/publican/protocol-to-docbook.xsl
>>> +++ b/doc/publican/protocol-to-docbook.xsl
>>> @@ -103,9 +103,22 @@
>>>      <listitem>
>>>          <simpara>
>>>            <xsl:if test="@enum">
>>> -            <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
>>> -              <xsl:value-of select="@enum"/>
>>> -            </link>
>>> +            <xsl:choose>
>>> +              <xsl:when test="contains(@enum, '.')">
>>> +                <link linkend="protocol-spec-{substring-before(@enum, '.')}-enum-{substring-after(@enum, '.')}">
>>> +                  <xsl:value-of select="substring-before(@enum, '.')"/>
>>> +                  <xsl:text>::</xsl:text>
>>> +                  <xsl:value-of select="substring-after(@enum, '.')"/>
>>> +                </link>
>>> +              </xsl:when>
>>> +              <xsl:otherwise>
>>> +                <link linkend="protocol-spec-{../../@name}-enum-{@enum}">
>>> +                  <xsl:value-of select="../../@name"/>
>>> +                  <xsl:text>::</xsl:text>
>>> +                  <xsl:value-of select="@enum"/>
>>> +                </link>
>>> +              </xsl:otherwise>
>>> +            </xsl:choose>
>>>           <xsl:text> </xsl:text>
>>>            </xsl:if>
>>>            <xsl:value-of select="@type"/>
>>
>> Hi,
>>
>> thanks for this.
>>
>> This XSL snippet does not work when the enum is defined in a different
>> XML file, does it? Just making sure I know what the expected behaviour
>> is.
>
> Correct; I don't think this is worth implementing at this stage.
>
>> Ok, there would be quite some things to polish, but since this patch
>> has waited for so long so I just rebased the xsl hunk myself, made some
>> whitespace fixes and pushed it:
>>    e21aeb5..7ccf35d  master -> master
>>
>> Please have a look at the merged patch in case I missed something.
>>
>> If you want to do more clean-ups, those can land after the alpha release.
>>
>>
>> Thanks,
>> pq
>
> The patch you pushed looks correct; thanks!