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

[Markus Armbruster]

Michael Roth <address@hidden> writes:

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

Yes, it's better to keep the two consistent.

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

Yes.