[Qemu-devel] [PATCH v2 1/2] scripts: use build_ prefix for string not piped through cgen()

[Qemu-devel] [PATCH v2 1/2] scripts: use build_ prefix for string not piped through cgen()

[Marc-André Lureau]

Suggested by Markus Armbruster:

The gen_ prefix is awkward.  Generated C should go through cgen()
exactly once (see commit 1f9a7a1).  The common way to get this wrong is
passing a foo=gen_foo() keyword argument to mcgen().  I'd like us to
adopt a naming convention where gen_ means "something that's been piped
through cgen(), and thus must not be passed to cgen() or mcgen()".
Requires renaming gen_params(), gen_marshal_proto() and
gen_event_send_proto().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/qapi.py          |  2 +-
 scripts/qapi-commands.py |  8 ++++----
 scripts/qapi-event.py    | 12 ++++++------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6c4d554165..d37a6157e0 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1884,7 +1884,7 @@ extern const char *const %(c_name)s_lookup[];
     return ret
 
 
-def gen_params(arg_type, boxed, extra):
+def build_params(arg_type, boxed, extra):
     if not arg_type:
         assert not boxed
         return extra
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 1943de4852..974d0a4a80 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -21,7 +21,7 @@ def gen_command_decl(name, arg_type, boxed, ret_type):
 ''',
                  c_type=(ret_type and ret_type.c_type()) or 'void',
                  c_name=c_name(name),
-                 params=gen_params(arg_type, boxed, 'Error **errp'))
+                 params=build_params(arg_type, boxed, 'Error **errp'))
 
 
 def gen_call(name, arg_type, boxed, ret_type):
@@ -82,7 +82,7 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
                  c_type=ret_type.c_type(), c_name=ret_type.c_name())
 
 
-def gen_marshal_proto(name):
+def build_marshal_proto(name):
     return ('void qmp_marshal_%s(QDict *args, QObject **ret, Error **errp)'
             % c_name(name))
 
@@ -91,7 +91,7 @@ def gen_marshal_decl(name):
     return mcgen('''
 %(proto)s;
 ''',
-                 proto=gen_marshal_proto(name))
+                 proto=build_marshal_proto(name))
 
 
 def gen_marshal(name, arg_type, boxed, ret_type):
@@ -103,7 +103,7 @@ def gen_marshal(name, arg_type, boxed, ret_type):
 {
     Error *err = NULL;
 ''',
-                proto=gen_marshal_proto(name))
+                proto=build_marshal_proto(name))
 
     if ret_type:
         ret += mcgen('''
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 0485e39145..bcbef1035f 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -14,10 +14,10 @@
 from qapi import *
 
 
-def gen_event_send_proto(name, arg_type, boxed):
+def build_event_send_proto(name, arg_type, boxed):
     return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
         'c_name': c_name(name.lower()),
-        'param': gen_params(arg_type, boxed, 'Error **errp')}
+        'param': build_params(arg_type, boxed, 'Error **errp')}
 
 
 def gen_event_send_decl(name, arg_type, boxed):
@@ -25,10 +25,10 @@ def gen_event_send_decl(name, arg_type, boxed):
 
 %(proto)s;
 ''',
-                 proto=gen_event_send_proto(name, arg_type, boxed))
+                 proto=build_event_send_proto(name, arg_type, boxed))
 
 
-# Declare and initialize an object 'qapi' using parameters from gen_params()
+# Declare and initialize an object 'qapi' using parameters from build_params()
 def gen_param_var(typ):
     assert not typ.variants
     ret = mcgen('''
@@ -42,7 +42,7 @@ def gen_param_var(typ):
         if memb.optional:
             ret += 'has_' + c_name(memb.name) + sep
         if memb.type.name == 'str':
-            # Cast away const added in gen_params()
+            # Cast away const added in build_params()
             ret += '(char *)'
         ret += c_name(memb.name)
     ret += mcgen('''
@@ -72,7 +72,7 @@ def gen_event_send(name, arg_type, boxed):
     Error *err = NULL;
     QMPEventFuncEmit emit;
 ''',
-                proto=gen_event_send_proto(name, arg_type, boxed))
+                proto=build_event_send_proto(name, arg_type, boxed))
 
     if arg_type and not arg_type.is_empty():
         ret += mcgen('''
-- 
2.13.0.91.g00982b8dd

[Qemu-devel] [PATCH v2 2/2] qapi: add location comment for generated types

[Marc-André Lureau]

This may help to find where the origin of the type was declared in the
json (when greping isn't easy enough).

Generates the following kind of C comment before types:

  /* /home/elmarco/src/qemu/qapi/introspect.json:94 */
  typedef struct SchemaInfo SchemaInfo;

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/qapi.py       | 12 +++++++++---
 scripts/qapi-event.py |  2 +-
 scripts/qapi-types.py | 18 +++++++++---------
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index d37a6157e0..7f9935eec0 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1852,15 +1852,21 @@ const char *const %(c_name)s_lookup[] = {
     return ret
 
 
-def gen_enum(name, values, prefix=None):
+def build_src_loc_comment(info):
+    if info:
+        return '\n/* %s:%d */' % (info['file'], info['line'])
+    else:
+        return ''
+
+def gen_enum(info, name, values, prefix=None):
     # append automatically generated _MAX value
     enum_values = values + ['_MAX']
 
     ret = mcgen('''
-
+%(comment)s
 typedef enum %(c_name)s {
 ''',
-                c_name=c_name(name))
+                c_name=c_name(name), comment=build_src_loc_comment(info))
 
     i = 0
     for value in enum_values:
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index bcbef1035f..cf5e282011 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -159,7 +159,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
         self._event_names = []
 
     def visit_end(self):
-        self.decl += gen_enum(event_enum_name, self._event_names)
+        self.decl += gen_enum(None, event_enum_name, self._event_names)
         self.defn += gen_enum_lookup(event_enum_name, self._event_names)
         self._event_names = None
 
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index b45e7b5634..9c8a3e277b 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -19,12 +19,12 @@ from qapi import *
 objects_seen = set()
 
 
-def gen_fwd_object_or_array(name):
+def gen_fwd_object_or_array(info, name):
     return mcgen('''
-
+%(comment)s
 typedef struct %(c_name)s %(c_name)s;
 ''',
-                 c_name=c_name(name))
+                 c_name=c_name(name), comment=build_src_loc_comment(info))
 
 
 def gen_array(name, element_type):
@@ -199,22 +199,22 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         # Special case for our lone builtin enum type
         # TODO use something cleaner than existence of info
         if not info:
-            self._btin += gen_enum(name, values, prefix)
+            self._btin += gen_enum(None, name, values, prefix)
             if do_builtins:
                 self.defn += gen_enum_lookup(name, values, prefix)
         else:
-            self._fwdecl += gen_enum(name, values, prefix)
+            self._fwdecl += gen_enum(info, name, values, prefix)
             self.defn += gen_enum_lookup(name, values, prefix)
 
     def visit_array_type(self, name, info, element_type):
         if isinstance(element_type, QAPISchemaBuiltinType):
-            self._btin += gen_fwd_object_or_array(name)
+            self._btin += gen_fwd_object_or_array(None, name)
             self._btin += gen_array(name, element_type)
             self._btin += gen_type_cleanup_decl(name)
             if do_builtins:
                 self.defn += gen_type_cleanup(name)
         else:
-            self._fwdecl += gen_fwd_object_or_array(name)
+            self._fwdecl += gen_fwd_object_or_array(info, name)
             self.decl += gen_array(name, element_type)
             self._gen_type_cleanup(name)
 
@@ -222,7 +222,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         # Nothing to do for the special empty builtin
         if name == 'q_empty':
             return
-        self._fwdecl += gen_fwd_object_or_array(name)
+        self._fwdecl += gen_fwd_object_or_array(info, name)
         self.decl += gen_object(name, base, members, variants)
         if base and not base.is_implicit():
             self.decl += gen_upcast(name, base)
@@ -233,7 +233,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
             self._gen_type_cleanup(name)
 
     def visit_alternate_type(self, name, info, variants):
-        self._fwdecl += gen_fwd_object_or_array(name)
+        self._fwdecl += gen_fwd_object_or_array(info, name)
         self.decl += gen_object(name, None, [variants.tag_member], variants)
         self._gen_type_cleanup(name)
 
-- 
2.13.0.91.g00982b8dd

Re: [Qemu-devel] [PATCH v2 1/2] scripts: use build_ prefix for string not piped through cgen()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

On 06/01/2017 07:41 AM, Marc-André Lureau wrote:

No 0/2 cover letter?

> Suggested by Markus Armbruster:
> 
> The gen_ prefix is awkward.  Generated C should go through cgen()
> exactly once (see commit 1f9a7a1).  The common way to get this wrong is
> passing a foo=gen_foo() keyword argument to mcgen().  I'd like us to
> adopt a naming convention where gen_ means "something that's been piped
> through cgen(), and thus must not be passed to cgen() or mcgen()".
> Requires renaming gen_params(), gen_marshal_proto() and
> gen_event_send_proto().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi.py          |  2 +-
>  scripts/qapi-commands.py |  8 ++++----
>  scripts/qapi-event.py    | 12 ++++++------
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] qapi: add location comment for generated types

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On 06/01/2017 07:41 AM, Marc-André Lureau wrote:
> This may help to find where the origin of the type was declared in the
> json (when greping isn't easy enough).
> 
> Generates the following kind of C comment before types:
> 
>   /* /home/elmarco/src/qemu/qapi/introspect.json:94 */
>   typedef struct SchemaInfo SchemaInfo;

Makes the generated code larger, but for a good cause.  No real impact
to the binary size (well, debug information may be slightly larger based
on line numbers getting higher, but that's in the noise).

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi.py       | 12 +++++++++---
>  scripts/qapi-event.py |  2 +-
>  scripts/qapi-types.py | 18 +++++++++---------
>  3 files changed, 19 insertions(+), 13 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] scripts: use build_ prefix for string not piped through cgen()

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Suggested by Markus Armbruster:

Replacing the line above with ...

> The gen_ prefix is awkward.  Generated C should go through cgen()
> exactly once (see commit 1f9a7a1).  The common way to get this wrong is
> passing a foo=gen_foo() keyword argument to mcgen().  I'd like us to
> adopt a naming convention where gen_ means "something that's been piped
> through cgen(), and thus must not be passed to cgen() or mcgen()".
> Requires renaming gen_params(), gen_marshal_proto() and
> gen_event_send_proto().

Suggested-by: Markus Armbruster <armbru@redhat.com>

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Applied, thanks!

Re: [Qemu-devel] [PATCH v2 2/2] qapi: add location comment for generated types

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> This may help to find where the origin of the type was declared in the
> json (when greping isn't easy enough).
>
> Generates the following kind of C comment before types:
>
>   /* /home/elmarco/src/qemu/qapi/introspect.json:94 */
>   typedef struct SchemaInfo SchemaInfo;

Can we do relative file names?  I.e. just qapi/introspect.json.

Would such comments be useful for things other than typedefs?

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi.py       | 12 +++++++++---
>  scripts/qapi-event.py |  2 +-
>  scripts/qapi-types.py | 18 +++++++++---------
>  3 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index d37a6157e0..7f9935eec0 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1852,15 +1852,21 @@ const char *const %(c_name)s_lookup[] = {
>      return ret
>  
>  
> -def gen_enum(name, values, prefix=None):
> +def build_src_loc_comment(info):
> +    if info:
> +        return '\n/* %s:%d */' % (info['file'], info['line'])

Leading instead of trailing newline, hmm.  Let's see how it works out.

> +    else:
> +        return ''

Let's drop the else: line.

> +
> +def gen_enum(info, name, values, prefix=None):

Make that name, info, values, ... for consistency with other functions
taking both name and info.  More of the same below.

>      # append automatically generated _MAX value
>      enum_values = values + ['_MAX']
>  
>      ret = mcgen('''
> -
> +%(comment)s

Loses the blank line when not info.

>  typedef enum %(c_name)s {
>  ''',
> -                c_name=c_name(name))
> +                c_name=c_name(name), comment=build_src_loc_comment(info))
>  
>      i = 0
>      for value in enum_values:
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index bcbef1035f..cf5e282011 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -159,7 +159,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>          self._event_names = []
>  
>      def visit_end(self):
> -        self.decl += gen_enum(event_enum_name, self._event_names)
> +        self.decl += gen_enum(None, event_enum_name, self._event_names)
>          self.defn += gen_enum_lookup(event_enum_name, self._event_names)
>          self._event_names = None
>  
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index b45e7b5634..9c8a3e277b 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -19,12 +19,12 @@ from qapi import *
>  objects_seen = set()
>  
>  
> -def gen_fwd_object_or_array(name):
> +def gen_fwd_object_or_array(info, name):
>      return mcgen('''
> -
> +%(comment)s

Likewise.

I think we should drop the newline from build_src_loc_comment()'s value,
and keep the blank line before its two uses.

>  typedef struct %(c_name)s %(c_name)s;
>  ''',
> -                 c_name=c_name(name))
> +                 c_name=c_name(name), comment=build_src_loc_comment(info))
>  
>  
>  def gen_array(name, element_type):
> @@ -199,22 +199,22 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>          # Special case for our lone builtin enum type
>          # TODO use something cleaner than existence of info
>          if not info:
> -            self._btin += gen_enum(name, values, prefix)
> +            self._btin += gen_enum(None, name, values, prefix)
>              if do_builtins:
>                  self.defn += gen_enum_lookup(name, values, prefix)
>          else:
> -            self._fwdecl += gen_enum(name, values, prefix)
> +            self._fwdecl += gen_enum(info, name, values, prefix)
>              self.defn += gen_enum_lookup(name, values, prefix)
>  
>      def visit_array_type(self, name, info, element_type):
>          if isinstance(element_type, QAPISchemaBuiltinType):
> -            self._btin += gen_fwd_object_or_array(name)
> +            self._btin += gen_fwd_object_or_array(None, name)
>              self._btin += gen_array(name, element_type)
>              self._btin += gen_type_cleanup_decl(name)
>              if do_builtins:
>                  self.defn += gen_type_cleanup(name)
>          else:
> -            self._fwdecl += gen_fwd_object_or_array(name)
> +            self._fwdecl += gen_fwd_object_or_array(info, name)
>              self.decl += gen_array(name, element_type)
>              self._gen_type_cleanup(name)
>  
> @@ -222,7 +222,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>          # Nothing to do for the special empty builtin
>          if name == 'q_empty':
>              return
> -        self._fwdecl += gen_fwd_object_or_array(name)
> +        self._fwdecl += gen_fwd_object_or_array(info, name)
>          self.decl += gen_object(name, base, members, variants)
>          if base and not base.is_implicit():
>              self.decl += gen_upcast(name, base)
> @@ -233,7 +233,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>              self._gen_type_cleanup(name)
>  
>      def visit_alternate_type(self, name, info, variants):
> -        self._fwdecl += gen_fwd_object_or_array(name)
> +        self._fwdecl += gen_fwd_object_or_array(info, name)
>          self.decl += gen_object(name, None, [variants.tag_member], variants)
>          self._gen_type_cleanup(name)

No comment gets generated before built-in types such as QType and
anyList.  That's okay, but perhaps the commit message could be a bit
more precise.

Re: [Qemu-devel] [PATCH v2 2/2] qapi: add location comment for generated types

[Marc-André Lureau]

Hi

On Thu, Jul 6, 2017 at 10:43 AM Markus Armbruster <armbru@redhat.com> wrote:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > This may help to find where the origin of the type was declared in the
> > json (when greping isn't easy enough).
> >
> > Generates the following kind of C comment before types:
> >
> >   /* /home/elmarco/src/qemu/qapi/introspect.json:94 */
> >   typedef struct SchemaInfo SchemaInfo;
>
> Can we do relative file names?  I.e. just qapi/introspect.json.
>

We could, but relative to $srcdir or $builddir? I think absolute path
avoids some potential confusion.

>
> Would such comments be useful for things other than typedefs?
>

Certainly it could, however I just needed it for typedef, didn't bother
adding it for the rest.


>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  scripts/qapi.py       | 12 +++++++++---
> >  scripts/qapi-event.py |  2 +-
> >  scripts/qapi-types.py | 18 +++++++++---------
> >  3 files changed, 19 insertions(+), 13 deletions(-)
> >
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index d37a6157e0..7f9935eec0 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -1852,15 +1852,21 @@ const char *const %(c_name)s_lookup[] = {
> >      return ret
> >
> >
> > -def gen_enum(name, values, prefix=None):
> > +def build_src_loc_comment(info):
> > +    if info:
> > +        return '\n/* %s:%d */' % (info['file'], info['line'])
>
> Leading instead of trailing newline, hmm.  Let's see how it works out.
>
> > +    else:
> > +        return ''
>
> Let's drop the else: line.
>

ok


>
> > +
> > +def gen_enum(info, name, values, prefix=None):
>
> Make that name, info, values, ... for consistency with other functions
> taking both name and info.  More of the same below.
>
>
ok


> >      # append automatically generated _MAX value
> >      enum_values = values + ['_MAX']
> >
> >      ret = mcgen('''
> > -
> > +%(comment)s
>
> Loses the blank line when not info.
>
> >  typedef enum %(c_name)s {
> >  ''',
> > -                c_name=c_name(name))
> > +                c_name=c_name(name), comment=build_src_loc_comment(
> info))
> >
> >      i = 0
> >      for value in enum_values:
> > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> > index bcbef1035f..cf5e282011 100644
> > --- a/scripts/qapi-event.py
> > +++ b/scripts/qapi-event.py
> > @@ -159,7 +159,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
> >          self._event_names = []
> >
> >      def visit_end(self):
> > -        self.decl += gen_enum(event_enum_name, self._event_names)
> > +        self.decl += gen_enum(None, event_enum_name, self._event_names)
> >          self.defn += gen_enum_lookup(event_enum_name,
> self._event_names)
> >          self._event_names = None
> >
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index b45e7b5634..9c8a3e277b 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -19,12 +19,12 @@ from qapi import *
> >  objects_seen = set()
> >
> >
> > -def gen_fwd_object_or_array(name):
> > +def gen_fwd_object_or_array(info, name):
> >      return mcgen('''
> > -
> > +%(comment)s
>
> Likewise.
>
> I think we should drop the newline from build_src_loc_comment()'s value,
> and keep the blank line before its two uses.
>
>
And you get an extra empty line if info is None. I don't mind. Other option
is to just add \n in the else case in build_src_loc_comment()


> >  typedef struct %(c_name)s %(c_name)s;
> >  ''',
> > -                 c_name=c_name(name))
> > +                 c_name=c_name(name), comment=build_src_loc_comment(
> info))
> >
> >
> >  def gen_array(name, element_type):
> > @@ -199,22 +199,22 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> >          # Special case for our lone builtin enum type
> >          # TODO use something cleaner than existence of info
> >          if not info:
> > -            self._btin += gen_enum(name, values, prefix)
> > +            self._btin += gen_enum(None, name, values, prefix)
> >              if do_builtins:
> >                  self.defn += gen_enum_lookup(name, values, prefix)
> >          else:
> > -            self._fwdecl += gen_enum(name, values, prefix)
> > +            self._fwdecl += gen_enum(info, name, values, prefix)
> >              self.defn += gen_enum_lookup(name, values, prefix)
> >
> >      def visit_array_type(self, name, info, element_type):
> >          if isinstance(element_type, QAPISchemaBuiltinType):
> > -            self._btin += gen_fwd_object_or_array(name)
> > +            self._btin += gen_fwd_object_or_array(None, name)
> >              self._btin += gen_array(name, element_type)
> >              self._btin += gen_type_cleanup_decl(name)
> >              if do_builtins:
> >                  self.defn += gen_type_cleanup(name)
> >          else:
> > -            self._fwdecl += gen_fwd_object_or_array(name)
> > +            self._fwdecl += gen_fwd_object_or_array(info, name)
> >              self.decl += gen_array(name, element_type)
> >              self._gen_type_cleanup(name)
> >
> > @@ -222,7 +222,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> >          # Nothing to do for the special empty builtin
> >          if name == 'q_empty':
> >              return
> > -        self._fwdecl += gen_fwd_object_or_array(name)
> > +        self._fwdecl += gen_fwd_object_or_array(info, name)
> >          self.decl += gen_object(name, base, members, variants)
> >          if base and not base.is_implicit():
> >              self.decl += gen_upcast(name, base)
> > @@ -233,7 +233,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> >              self._gen_type_cleanup(name)
> >
> >      def visit_alternate_type(self, name, info, variants):
> > -        self._fwdecl += gen_fwd_object_or_array(name)
> > +        self._fwdecl += gen_fwd_object_or_array(info, name)
> >          self.decl += gen_object(name, None, [variants.tag_member],
> variants)
> >          self._gen_type_cleanup(name)
>
> No comment gets generated before built-in types such as QType and
> anyList.  That's okay, but perhaps the commit message could be a bit
> more precise.
>
>
ok

Re: [Qemu-devel] [PATCH v2 2/2] qapi: add location comment for generated types

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Thu, Jul 6, 2017 at 10:43 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > This may help to find where the origin of the type was declared in the
>> > json (when greping isn't easy enough).
>> >
>> > Generates the following kind of C comment before types:
>> >
>> >   /* /home/elmarco/src/qemu/qapi/introspect.json:94 */
>> >   typedef struct SchemaInfo SchemaInfo;
>>
>> Can we do relative file names?  I.e. just qapi/introspect.json.
>>
>
> We could, but relative to $srcdir or $builddir? I think absolute path
> avoids some potential confusion.

Source files are always relative to $srcdir.

Absolute paths are a total pain when you diff files generated in
separate trees.

>> Would such comments be useful for things other than typedefs?
>>
>
> Certainly it could, however I just needed it for typedef, didn't bother
> adding it for the rest.

Fair enough.

>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  scripts/qapi.py       | 12 +++++++++---
>> >  scripts/qapi-event.py |  2 +-
>> >  scripts/qapi-types.py | 18 +++++++++---------
>> >  3 files changed, 19 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/scripts/qapi.py b/scripts/qapi.py
>> > index d37a6157e0..7f9935eec0 100644
>> > --- a/scripts/qapi.py
>> > +++ b/scripts/qapi.py
>> > @@ -1852,15 +1852,21 @@ const char *const %(c_name)s_lookup[] = {
>> >      return ret
>> >
>> >
>> > -def gen_enum(name, values, prefix=None):
>> > +def build_src_loc_comment(info):
>> > +    if info:
>> > +        return '\n/* %s:%d */' % (info['file'], info['line'])
>>
>> Leading instead of trailing newline, hmm.  Let's see how it works out.
>>
>> > +    else:
>> > +        return ''
>>
>> Let's drop the else: line.
>>
>
> ok
>
>
>>
>> > +
>> > +def gen_enum(info, name, values, prefix=None):
>>
>> Make that name, info, values, ... for consistency with other functions
>> taking both name and info.  More of the same below.
>>
>>
> ok
>
>
>> >      # append automatically generated _MAX value
>> >      enum_values = values + ['_MAX']
>> >
>> >      ret = mcgen('''
>> > -
>> > +%(comment)s
>>
>> Loses the blank line when not info.
>>
>> >  typedef enum %(c_name)s {
>> >  ''',
>> > -                c_name=c_name(name))
>> > +                c_name=c_name(name), comment=build_src_loc_comment(info))
>> >
>> >      i = 0
>> >      for value in enum_values:
>> > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
>> > index bcbef1035f..cf5e282011 100644
>> > --- a/scripts/qapi-event.py
>> > +++ b/scripts/qapi-event.py
>> > @@ -159,7 +159,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>> >          self._event_names = []
>> >
>> >      def visit_end(self):
>> > -        self.decl += gen_enum(event_enum_name, self._event_names)
>> > +        self.decl += gen_enum(None, event_enum_name, self._event_names)
>> >          self.defn += gen_enum_lookup(event_enum_name, self._event_names)
>> >          self._event_names = None
>> >
>> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> > index b45e7b5634..9c8a3e277b 100644
>> > --- a/scripts/qapi-types.py
>> > +++ b/scripts/qapi-types.py
>> > @@ -19,12 +19,12 @@ from qapi import *
>> >  objects_seen = set()
>> >
>> >
>> > -def gen_fwd_object_or_array(name):
>> > +def gen_fwd_object_or_array(info, name):
>> >      return mcgen('''
>> > -
>> > +%(comment)s
>>
>> Likewise.
>>
>> I think we should drop the newline from build_src_loc_comment()'s value,
>> and keep the blank line before its two uses.
>>
>>
> And you get an extra empty line if info is None. I don't mind.

I misread your code.  The leading '\n' is kind of weird, but it works.

>                                                                Other option
> is to just add \n in the else case in build_src_loc_comment()

Could work, too.  Perhaps even something like '/* built-in */\n'.

[...]