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 <marcandre.lur...@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).
An example for the generated comment would be nice here.
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> Cc: qemu-triv...@nongnu.org
> ---
> 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)
