[v2,2/2] qapi: add location comment for generated types

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(-)

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>

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.

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

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'.

[...]