Re: [Qemu-devel] [PATCH 01/21] qapi: add info comment for generated types

[Markus Armbruster]

Re qemu-trivial: since I got another lengthy series touching
scripts/qapi* in the pipeline, I'd prefer to handle the conflicts in my
tree.

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

An example for the generated comment would be nice here.

> Signed-off-by: Marc-André Lureau <address@hidden>
> Cc: address@hidden
> ---
>  scripts/qapi.py       | 11 +++++++++--
>  scripts/qapi-event.py |  4 +++-
>  scripts/qapi-types.py | 17 +++++++++--------
>  3 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 53a44779d0..9504ebd8c7 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1874,15 +1874,22 @@ const char *const %(c_name)s_lookup[] = {
>      return ret
>  
>  
> -def gen_enum(name, values, prefix=None):
> +def gen_info_comment(info):

Well, aren't all comments "info comments"?  What distinguishes this one
is that it's about the location of the source.  Suggest to rename to
gen_loc_comment() or maybe gen_src_loc_comment().

Hmm, 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().

> +    if info:
> +        return "/* %s:%d */" % (info['file'], info['line'])
> +    else:
> +        return ""

Please stick to the prevalent use of single vs. double quotes: use
single quotes unless double quotes let you avoid backslashes, except
always use double quotes for error messages.  See "[PATCH for-2.9 19/47]
qapi: Prefer single-quoted strings more consistently".

> +
> +def gen_enum(info, name, values, prefix=None):
>      # append automatically generated _MAX value
>      enum_values = values + ['_MAX']
>  
>      ret = mcgen('''
>  
> +%(info)s
>  typedef enum %(c_name)s {
>  ''',
> -                c_name=c_name(name))
> +                c_name=c_name(name), info=gen_info_comment(info))

Your chance to use identifier infocom ;-P

Seriously, I'd prefer comment to info.

>  
>      i = 0
>      for value in enum_values:
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index f4eb7f85b1..ca90d6a5df 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -152,6 +152,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>          self.decl = None
>          self.defn = None
>          self._event_names = None
> +        self.info = None
>  
>      def visit_begin(self, schema):
>          self.decl = ''
> @@ -159,7 +160,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(self.info, event_enum_name, self._event_names)
>          self.defn += gen_enum_lookup(event_enum_name, self._event_names)
>          self._event_names = None
>  
> @@ -167,6 +168,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>          self.decl += gen_event_send_decl(name, arg_type, boxed)
>          self.defn += gen_event_send(name, arg_type, boxed)
>          self._event_names.append(name)
> +        self.info = info
>  
>  

I'm afraid this doesn't make sense.

You set self.info in each visit_event(), then use it in visit_end().
visit_end() thus sees the info of whatever event was visited last.
Won't produce a sane location comment.  None of the others would,
either.  That's because the enumeration is defined implicitly by all the
events together.

Let's pass info=None for this enum.

>  (input_file, output_dir, do_c, do_h, prefix, dummy) = parse_command_line()
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index dabc42e047..896749bf61 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -19,12 +19,13 @@ from qapi import *
>  objects_seen = set()
>  
>  
> -def gen_fwd_object_or_array(name):
> +def gen_fwd_object_or_array(info, name):
>      return mcgen('''
>  
> +%(info)s
>  typedef struct %(c_name)s %(c_name)s;

If info=None, gen_info_comment(info) is '', and we get an unwanted blank
line.

>  ''',
> -                 c_name=c_name(name))
> +                 c_name=c_name(name), info=gen_info_comment(info))
>  
>  
>  def gen_array(name, element_type):
> @@ -199,22 +200,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(info, name, values, prefix)

gen_enum(None, ...) feels slightly clearer to me.

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

Note that @info points to an arbitrarily chosen user of the array type,
*not* to the definition of the element type.  The resulting source
location comment feels useless to me.  Suggest to pass None for arrays.

> @@ -222,7 +223,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 +234,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)