[Spice-devel] [PATCH spice-common v2] codegen: Define identity macros for enum types

[Frediano Ziglio]

> 
> One side effect of this patch is that it makes enums.h fairly
> unreadable, which is not so good as it's exposed as public API.
> 

I think this header should not take as reference. It's an auto generated
file which had not proper comments, people should read spice.proto.

> Do you expect to use this that often? I agree it's convenient, but on
> the other hand, it's going to make it fairly easy to make "broken"
> builds if this is used a lot in the code, as some features will be
> silently disabled if a too old spice-protocol happens to be installed.

This is true. Was already considering this, not clear idea if this
could be considered better or worst.

> The availability of enum members is not very hard to check at configure
> time, and since we bump spice-protocol version after release, a version
> check could be enough.
> 

On the other way it ends up with the same problem, silently disable
some piece of code. Unless the check is something like

#if SPICE_PROTOCOL_VERSION > 0.12.14 || defined(STUFF_I_WANT)
#endif

in this way program will stop compiling when the new version, supposed
to support this will came out. I suppose you can achieve this also
checking enum values present in configure and assuming is present if we
get some spice-protocol version and checking for macro from configure
in the code. That is moving the check to configure.ac and not having
identity macros in enums.h.
This solution has the drawback to have more complex configure.ac
which should be updated every time a new spice-protocol release
is out. But this was also requires to remove the conditional
check on the source.

I'll try to implement the "Tell client we are just streaming data"
patch using more configure.ac "magic", just to have a comparison.

> Christophe
> 
> 
> 
> On Thu, Nov 16, 2017 at 12:48:14PM +0000, Frediano Ziglio wrote:
> > An identity macro in C is a macro defined as
> > 
> >   #define SOME_NAME SOME_NAME
> > 
> > They are often used to make possible to use preprocessor to check
> > for values existence like
> > 
> >   #ifdef SOME_NAME
> >   ... use SOME_NAME ...
> >   #endif
> > 
> > Defining these macro for each enumeration value allows to do finer
> > check in the code that uses spice-protocol.
> > 
> > More specifically you could check a message existence with
> > 
> >   #ifdef SPICE_MSG_MAIN_CHANNELS_LIST
> >   ... use channels_list message ..
> >   #endif
> > 
> > or if a given channel exists with
> > 
> >   #ifdef SPICE_CHANNEL_WEBDAV
> >   ... use WebDAVChannel ...
> >   #endif
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  python_modules/codegen.py | 10 ++++++++++
> >  python_modules/ptypes.py  |  2 ++
> >  spice_codegen.py          |  2 ++
> >  3 files changed, 14 insertions(+)
> > 
> > Changes since v1:
> > - better commit message.
> > 
> > diff --git a/python_modules/codegen.py b/python_modules/codegen.py
> > index f7a2048..6143905 100644
> > --- a/python_modules/codegen.py
> > +++ b/python_modules/codegen.py
> > @@ -327,6 +327,16 @@ class CodeWriter:
> >      def macro(self, name, args, define):
> >          self.write("#define %s(%s) %s" % (name, args, define)).newline()
> >  
> > +    def identity_macro(self, name):
> > +        """Define a preprocessor macro with same name as value.
> > +        This helps writing conditional code using preprocessor defines.
> > +        """
> > +        assert(self.at_line_start)
> > +        indentation = self.indentation
> > +        self.indentation = 0
> > +        self.writeln("#define %s %s" % (name, name))
> > +        self.indentation = indentation
> > +
> >      def ifdef(self, name):
> >          indentation = self.indentation
> >          self.indentation = 0;
> > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > index 63a7a2f..47b32b5 100644
> > --- a/python_modules/ptypes.py
> > +++ b/python_modules/ptypes.py
> > @@ -381,6 +381,7 @@ class EnumType(EnumBaseType):
> >                  writer.write(" = %d" % (i))
> >              writer.write(",")
> >              writer.newline()
> > +            writer.identity_macro(self.c_enumname(i))
> >              current_default = i + 1
> >          writer.newline()
> >          writer.write(codegen.prefix_underscore_upper(self.name.upper(),
> >          "ENUM_END"))
> > @@ -434,6 +435,7 @@ class FlagsType(EnumBaseType):
> >              writer.write(" = (1 << %d)" % (i))
> >              writer.write(",")
> >              writer.newline()
> > +            writer.identity_macro(self.c_enumname(i))
> >              current_default = i + 1
> >          writer.newline()
> >          writer.write(codegen.prefix_underscore_upper(self.name.upper(),
> >          "MASK"))
> > diff --git a/spice_codegen.py b/spice_codegen.py
> > index 76d7c5e..bb50f31 100755
> > --- a/spice_codegen.py
> > +++ b/spice_codegen.py
> > @@ -40,6 +40,7 @@ def write_channel_enums(writer, channel, client,
> > describe):
> >              else:
> >                  writer.writeln("%s = %s," % (enum, m.value))
> >                  i = m.value + 1
> > +            writer.identity_macro(enum)
> >      if describe:
> >          writer.writeln("{ 0, NULL }");
> >      else:
> > @@ -68,6 +69,7 @@ def write_channel_type_enum(writer, describe=False):
> >              else:
> >                  writer.writeln("%s = %s," % (enum, c.value))
> >                  i = c.value + 1
> > +            writer.identity_macro(enum)
> >      writer.newline()
> >      if describe:
> >          writer.writeln("{ 0, NULL }")
> > --
> > 2.13.6
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>