[Spice-devel] [PATCH spice-server] syntax-check: Use templates for glib-mkenums

[Christophe Fergeau]

On Mon, Nov 14, 2016 at 12:18:13PM -0500, Frediano Ziglio wrote:
> > On Mon, Nov 14, 2016 at 12:46:26PM +0000, Frediano Ziglio wrote:
> > > Syntax checker complained about autoconf variable expansion used
> > > inside Makefile.am.
> > > This patch uses template files instead of options.
> > > This also reduces quoting making template code more readable.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/Makefile.am               | 34 +++++++------------------------
> > >  server/spice-server-enums.tmpl.c | 43
> > >  ++++++++++++++++++++++++++++++++++++++++
> > >  server/spice-server-enums.tmpl.h | 23 +++++++++++++++++++++
> > >  3 files changed, 73 insertions(+), 27 deletions(-)
> > >  create mode 100644 server/spice-server-enums.tmpl.c
> > >  create mode 100644 server/spice-server-enums.tmpl.h
> > > 
> > > diff --git a/server/Makefile.am b/server/Makefile.am
> > > index 91f9666..5d1e9ee 100644
> > > --- a/server/Makefile.am
> > > +++ b/server/Makefile.am
> > > @@ -201,33 +201,11 @@ endif
> > >  libspice_server_la_LIBADD = libserver.la
> > >  libspice_server_la_SOURCES =
> > >  
> > > -spice-server-enums.c: spice-server.h
> > > -	$(AM_V_GEN)glib-mkenums --fhead "#include \"config.h\"\n\n" \
> > > -			--fhead "#include <glib-object.h>\n" \
> > > -			--fhead "#include \"spice-server-enums.h\"\n\n" \
> > > -			--fprod "\n#include \"spice-server.h\"\n" \
> > > -			--vhead "static const G at Type@Value _ at enum_name@_values[] = {" \
> > > -			--vprod "  { @VALUENAME@, \"@VALUENAME@\", \"@valuenick@\" }," \
> > > -			--vtail "  { 0, NULL, NULL }\n};\n\n" \
> > > -			--vtail "GType\n at enum_name@_get_type (void)\n{\n" \
> > > -			--vtail "  static GType type = 0;\n" \
> > > -			--vtail "  static volatile gsize type_volatile = 0;\n\n" \
> > > -			--vtail "  if (g_once_init_enter(&type_volatile)) {\n" \
> > > -			--vtail "    type = g_ at type@_register_static (\"@EnumName@\",
> > > _ at enum_name@_values);\n" \
> > > -			--vtail "    g_once_init_leave(&type_volatile, type);\n" \
> > > -			--vtail "  }\n\n" \
> > > -			--vtail "  return type;\n}\n\n" \
> > > -		$^ > $@
> > > -
> > > -spice-server-enums.h: spice-server.h
> > > -	$(AM_V_GEN)glib-mkenums --fhead "#ifndef SPICE_SERVER_ENUMS_H\n" \
> > > -			--fhead "#define SPICE_SERVER_ENUMS_H\n\n" \
> > > -			--fhead "G_BEGIN_DECLS\n\n" \
> > > -			--ftail "G_END_DECLS\n\n" \
> > > -			--ftail "#endif /* SPICE_SERVER_ENUMS_H */\n" \
> > > -			--eprod "#define SPICE_TYPE_ at ENUMSHORT@ @enum_name at _get_type()\n" \
> > > -			--eprod "GType @enum_name at _get_type (void);\n" \
> > > -		$^ >  $@
> > > +spice-server-enums.c: spice-server.h spice-server-enums.tmpl.c
> > > +	$(AM_V_GEN)glib-mkenums --template spice-server-enums.tmpl.c $^ > $@
> > > +
> > > +spice-server-enums.h: spice-server.h spice-server-enums.tmpl.h
> > > +	$(AM_V_GEN)glib-mkenums --template spice-server-enums.tmpl.h $^ > $@
> > >  
> > >  EXTRA_DIST =					\
> > >  	spice-bitmap-utils.tmpl.c			\
> > > @@ -235,6 +213,8 @@ EXTRA_DIST =					\
> > >  	glz-encode-match.tmpl.c			\
> > >  	glz-encode.tmpl.c			\
> > >  	spice-server.syms			\
> > > +	spice-server-enums.tmpl.h		\
> > > +	spice-server-enums.tmpl.c		\
> > >  	$(NULL)
> > >  
> > >  BUILT_SOURCES = $(spice_built_sources)
> > > diff --git a/server/spice-server-enums.tmpl.c
> > > b/server/spice-server-enums.tmpl.c
> > > new file mode 100644
> > > index 0000000..5e7fec3
> > > --- /dev/null
> > > +++ b/server/spice-server-enums.tmpl.c
> > > @@ -0,0 +1,43 @@
> > > +/*** BEGIN file-header ***/
> > > +#ifdef HAVE_CONFIG_H
> > > +#include <config.h>
> > > +#endif
> > > +
> > > +#include <glib-object.h>
> > > +
> > > +#include "spice-server-enums.h"
> > > +
> > > +/*** END file-header ***/
> > > +
> > > +/*** BEGIN file-production ***/
> > > +#include "spice-server.h"
> > 
> > If you feel like it, you can move this one with the other headers
> > 
> 
> I tried to be conservative (replicate the same behavior of
> the Makefile).
> Maybe can even be removed.
> But I would prefer to do in a separate patch.

Just keep it that way :) But the <config.h> inclusion is also changed in
this patch, so before/after version is not 100% identical.
> > > diff --git a/server/spice-server-enums.tmpl.h
> > > b/server/spice-server-enums.tmpl.h
> > > new file mode 100644
> > > index 0000000..57098d4
> > > --- /dev/null
> > > +++ b/server/spice-server-enums.tmpl.h
> > > @@ -0,0 +1,23 @@
> > > +/*** BEGIN file-header ***/
> > > +#ifndef SPICE_SERVER_ENUMS_H
> > > +#define SPICE_SERVER_ENUMS_H
> > > +
> > > +G_BEGIN_DECLS
> > > +
> > > +/*** END file-header ***/
> > > +
> > > +/*** BEGIN file-production ***/
> > > +#include "spice-server.h"
> > 
> > Was this include needed? I don't think it was there before.
> > 
> 
> Probably is not needed in the .c file as it's including the
> .h one.

My point was mostly that the .h did not have the include before I think.
But just mentioning it, feel free to keep things this way.

> 
> > Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> > 
> > Christophe
> > 
> 
> Question about file naming... usually the spice-*.h are public
> headers, should we use some different names?

spice-bitmap-utils.h also uses the 'spice' prefix. These enums are
generated from a public header, so I think it's acceptable to keep
'spice'. I'm also ok with red-enums.h if you prefer.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161114/22f34834/attachment.sig>