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

[Markus Armbruster]

Marc-André Lureau <address@hidden> 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 <address@hidden>
> ---
>  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.