Re: [Qemu-devel] [PATCH v2 05/29] qapi: New classes QAPIGenC, QAPIGenH, QAPIGenDoc

[Michael Roth]

Quoting Markus Armbruster (2018-02-11 03:35:43)
> These classes encapsulate accumulating and writing output.
> 
> Convert C code generation to QAPIGenC and QAPIGenH.  The conversion is
> rather shallow: most of the output accumulation is not converted.
> Left for later.
> 
> The indentation machinery uses a single global variable indent_level,
> even though we generally interleave creation of a .c and its .h.  It
> should become instance variable of QAPIGenC.  Also left for later.
> 
> Documentation generation isn't converted, and QAPIGenDoc isn't used.
> This will change shortly.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>

2 minor nits below, but in any case:

Reviewed-by: Michael Roth <address@hidden>

> ---
>  scripts/qapi-commands.py   | 23 +++++------
>  scripts/qapi-event.py      | 22 ++++++-----
>  scripts/qapi-introspect.py | 18 +++++----
>  scripts/qapi-types.py      | 22 ++++++-----
>  scripts/qapi-visit.py      | 22 ++++++-----
>  scripts/qapi.py            | 99 
> +++++++++++++++++++++++++---------------------
>  6 files changed, 112 insertions(+), 94 deletions(-)
> 
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index c3aa52fce1..8d38ade076 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -260,12 +260,10 @@ blurb = '''
>   * Schema-defined QAPI/QMP commands
>  '''
> 
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -                            'qmp-marshal.c', 'qmp-commands.h',
> -                            blurb, __doc__)
> -
> -fdef.write(mcgen('''
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
> 
> +genc.add(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/module.h"
> @@ -280,20 +278,23 @@ fdef.write(mcgen('''
>  #include "%(prefix)sqmp-commands.h"
> 
>  ''',
> -                 prefix=prefix))
> +               prefix=prefix))
> 
> -fdecl.write(mcgen('''
> +genh.add(mcgen('''
>  #include "%(prefix)sqapi-types.h"
>  #include "qapi/qmp/dispatch.h"
> 
>  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>  ''',
> -                  prefix=prefix, c_prefix=c_name(prefix, protect=False)))
> +               prefix=prefix, c_prefix=c_name(prefix, protect=False)))
> 
>  schema = QAPISchema(input_file)
>  vis = QAPISchemaGenCommandVisitor()
>  schema.visit(vis)
> -fdef.write(vis.defn)
> -fdecl.write(vis.decl)
> +genc.add(vis.defn)
> +genh.add(vis.decl)
> 
> -close_output(fdef, fdecl)
> +if do_c:
> +    genc.write(output_dir, prefix + 'qmp-marshal.c')
> +if do_h:
> +    genh.write(output_dir, prefix + 'qmp-commands.h')
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index edb9ddb650..bd7a9be3dc 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -176,11 +176,10 @@ blurb = '''
>   * Schema-defined QAPI/QMP events
>  '''
> 
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -                            'qapi-event.c', 'qapi-event.h',
> -                            blurb, __doc__)
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
> 
> -fdef.write(mcgen('''
> +genc.add(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "%(prefix)sqapi-event.h"
> @@ -191,21 +190,24 @@ fdef.write(mcgen('''
>  #include "qapi/qmp-event.h"
> 
>  ''',
> -                 prefix=prefix))
> +               prefix=prefix))
> 
> -fdecl.write(mcgen('''
> +genh.add(mcgen('''
>  #include "qapi/util.h"
>  #include "%(prefix)sqapi-types.h"
> 
>  ''',
> -                  prefix=prefix))
> +               prefix=prefix))
> 
>  event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
> 
>  schema = QAPISchema(input_file)
>  vis = QAPISchemaGenEventVisitor()
>  schema.visit(vis)
> -fdef.write(vis.defn)
> -fdecl.write(vis.decl)
> +genc.add(vis.defn)
> +genh.add(vis.decl)
> 
> -close_output(fdef, fdecl)
> +if do_c:
> +    genc.write(output_dir, prefix + 'qapi-event.c')
> +if do_h:
> +    genh.write(output_dir, prefix + 'qapi-event.h')
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index ebe8706f41..3d65690fe3 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -181,21 +181,23 @@ blurb = '''
>   * QAPI/QMP schema introspection
>  '''
> 
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -                            'qmp-introspect.c', 'qmp-introspect.h',
> -                            blurb, __doc__)
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
> 
> -fdef.write(mcgen('''
> +genc.add(mcgen('''
>  #include "qemu/osdep.h"
>  #include "%(prefix)sqmp-introspect.h"
> 
>  ''',
> -                 prefix=prefix))
> +               prefix=prefix))
> 
>  schema = QAPISchema(input_file)
>  vis = QAPISchemaGenIntrospectVisitor(opt_unmask)
>  schema.visit(vis)
> -fdef.write(vis.defn)
> -fdecl.write(vis.decl)
> +genc.add(vis.defn)
> +genh.add(vis.decl)
> 
> -close_output(fdef, fdecl)
> +if do_c:
> +    genc.write(output_dir, prefix + 'qmp-introspect.c')
> +if do_h:
> +    genh.write(output_dir, prefix + 'qmp-introspect.h')
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4db8424da1..c0ac879beb 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -180,7 +180,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>          self.decl = ''
>          self.defn = ''
>          self._fwdecl = ''
> -        self._btin = guardstart('QAPI_TYPES_BUILTIN')
> +        self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')

Minor nit, but if we compensate for guardstart() change here, shouldn't
we do the same in QAPISchemaGenVisitVisitor? Both are cosmetic (though
Visit is in less need since it has extra an extra newline already,
but changing one and not the other to compensate here makes the patch
appear less mechanical, and the resulting formatting fix-up gets dropped
later in the series anyway)

> 
>      def visit_end(self):
>          self.decl = self._fwdecl + self.decl

<snip>

> +class QAPIGenDoc(QAPIGen):
> +    def _top(self, fname):
> +        return (QAPIGen._top(self, fname)
> +                + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')

The whitespace change in
"qapi/types qapi/visit: Generate built-in stuff into separate files" should
probably be squashed in here:

 class QAPIGenDoc(QAPIGen):
+
     def _top(self, fname):
         return (QAPIGen._top(self, fname)
                 + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')

> -- 
> 2.13.6
>