Re: [Qemu-devel] [PATCH RFC 03/21] qapi: New classes QAPIGenC, QAPIGenH, QAPIGenDoc

[Markus Armbruster]

Marc-Andre Lureau <address@hidden> writes:

> Hi
>
> On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster <address@hidden> wrote:
>> 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>
>> ---
>>  scripts/qapi-commands.py   | 27 ++++++-------
>>  scripts/qapi-event.py      | 26 +++++++------
>>  scripts/qapi-introspect.py | 22 ++++++-----
>>  scripts/qapi-types.py      | 26 +++++++------
>>  scripts/qapi-visit.py      | 26 +++++++------
>>  scripts/qapi.py            | 96 
>> ++++++++++++++++++++++++++--------------------
>>  6 files changed, 122 insertions(+), 101 deletions(-)
>>
>> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
>> index a861ac52e7..4be7dbc482 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.body(mcgen('''
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>>  #include "qemu/module.h"
>> @@ -279,21 +277,24 @@ fdef.write(mcgen('''
>>  #include "%(prefix)sqmp-commands.h"
>>
>>  ''',
>> -                 prefix=prefix))
>> +                prefix=prefix))
>>
>> -fdecl.write(mcgen('''
>> +genh.body(mcgen('''
>>  #include "%(prefix)sqapi-types.h"
>>  #include "qapi/qmp/qdict.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)
>> -gen = QAPISchemaGenCommandVisitor()
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenCommandVisitor()
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(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 b1d611c5ea..da3de17c76 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.body(mcgen('''
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>>  #include "%(prefix)sqapi-event.h"
>> @@ -190,22 +189,25 @@ fdef.write(mcgen('''
>>  #include "qapi/qmp-event.h"
>>
>>  ''',
>> -                 prefix=prefix))
>> +                prefix=prefix))
>>
>> -fdecl.write(mcgen('''
>> +genh.body(mcgen('''
>>  #include "qapi/util.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "%(prefix)sqapi-types.h"
>>
>>  ''',
>> -                  prefix=prefix))
>> +                prefix=prefix))
>>
>>  event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
>>
>>  schema = QAPISchema(input_file)
>> -gen = QAPISchemaGenEventVisitor()
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenEventVisitor()
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(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 bd9253a172..c654f8fa94 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.body(mcgen('''
>>  #include "qemu/osdep.h"
>>  #include "%(prefix)sqmp-introspect.h"
>>
>>  ''',
>> -                 prefix=prefix))
>> +                prefix=prefix))
>>
>>  schema = QAPISchema(input_file)
>> -gen = QAPISchemaGenIntrospectVisitor(opt_unmask)
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenIntrospectVisitor(opt_unmask)
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(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 1103dbda2d..97406b3368 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')
>>
>>      def visit_end(self):
>>          self.decl = self._fwdecl + self.decl
>> @@ -256,26 +256,28 @@ blurb = '''
>>   * Schema-defined QAPI types
>>  '''
>>
>> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>> -                            'qapi-types.c', 'qapi-types.h',
>> -                            blurb, __doc__)
>> +genc = QAPIGenC(blurb, __doc__)
>> +genh = QAPIGenH(blurb, __doc__)
>>
>> -fdef.write(mcgen('''
>> +genc.body(mcgen('''
>>  #include "qemu/osdep.h"
>>  #include "qapi/dealloc-visitor.h"
>>  #include "%(prefix)sqapi-types.h"
>>  #include "%(prefix)sqapi-visit.h"
>>  ''',
>> -                 prefix=prefix))
>> +                prefix=prefix))
>>
>> -fdecl.write(mcgen('''
>> +genh.body(mcgen('''
>>  #include "qapi/util.h"
>>  '''))
>>
>>  schema = QAPISchema(input_file)
>> -gen = QAPISchemaGenTypeVisitor()
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenTypeVisitor()
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(vis.decl)
>>
>> -close_output(fdef, fdecl)
>> +if do_c:
>> +    genc.write(output_dir, prefix + 'qapi-types.c')
>> +if do_h:
>> +    genh.write(output_dir, prefix + 'qapi-types.h')
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index 5231f89c36..d1b25daf0d 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -339,30 +339,32 @@ blurb = '''
>>   * Schema-defined QAPI visitors
>>  '''
>>
>> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>> -                            'qapi-visit.c', 'qapi-visit.h',
>> -                            blurb, __doc__)
>> +genc = QAPIGenC(blurb, __doc__)
>> +genh = QAPIGenH(blurb, __doc__)
>>
>> -fdef.write(mcgen('''
>> +genc.body(mcgen('''
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>>  #include "qapi/error.h"
>>  #include "%(prefix)sqapi-visit.h"
>>  ''',
>> -                 prefix=prefix))
>> +                prefix=prefix))
>>
>> -fdecl.write(mcgen('''
>> +genh.body(mcgen('''
>>  #include "qapi/visitor.h"
>>  #include "qapi/qmp/qerror.h"
>>  #include "%(prefix)sqapi-types.h"
>>
>>  ''',
>> -                  prefix=prefix))
>> +                prefix=prefix))
>>
>>  schema = QAPISchema(input_file)
>> -gen = QAPISchemaGenVisitVisitor()
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenVisitVisitor()
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(vis.decl)
>>
>> -close_output(fdef, fdecl)
>> +if do_c:
>> +    genc.write(output_dir, prefix + 'qapi-visit.c')
>> +if do_h:
>> +    genh.write(output_dir, prefix + 'qapi-visit.h')
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index d0816f7479..d73ef618e2 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -2,7 +2,7 @@
>>  # QAPI helper library
>>  #
>>  # Copyright IBM, Corp. 2011
>> -# Copyright (c) 2013-2016 Red Hat Inc.
>> +# Copyright (c) 2013-2018 Red Hat Inc.
>>  #
>>  # Authors:
>>  #  Anthony Liguori <address@hidden>
>> @@ -1820,7 +1820,6 @@ def guardname(filename):
>>
>>  def guardstart(name):
>>      return mcgen('''
>> -
>>  #ifndef %(name)s
>>  #define %(name)s
>>
>> @@ -1832,7 +1831,6 @@ def guardend(name):
>>      return mcgen('''
>>
>>  #endif /* %(name)s */
>> -
>>  ''',
>>                   name=guardname(name))
>>
>> @@ -1970,17 +1968,53 @@ def parse_command_line(extra_options='', 
>> extra_long_options=[]):
>>
>>      return (fname, output_dir, do_c, do_h, prefix, extra_opts)
>>
>> +
>>  #
>> -# Generate output files with boilerplate
>> +# Accumulate and write output
>>  #
>>
>> +class QAPIGen(object):
>> +
>> +    def __init__(self):
>> +        self._preamble = ''
>> +        self._body = ''
>> +
>> +    def preamble(self, text):
>> +        self._preamble += text
>> +
>> +    def body(self, text):
>> +        self._body += text
>> +
>> +    def top(self, fname):
>> +        return ''
>> +
>> +    def bottom(self, fname):
>> +        return ''
>> +
>
> Some methods appends, other return. That's a bit confusing. Why not
> name them accordingly? add_preamble, add_body, get_top...?

You're right, the functions called for side effects have less than
obvious names.  I think I'll keep the noun names for the pure functions.

>> +    def write(self, output_dir, fname):
>> +        if output_dir:
>> +            try:
>> +                os.makedirs(output_dir)
>> +            except os.error as e:
>> +                if e.errno != errno.EEXIST:
>> +                    raise
>> +        f = open(os.path.join(output_dir, fname), 'w')
>> +        f.write(self.top(fname) + self._preamble + self._body
>> +                + self.bottom(fname))
>> +        f.close()
>> +
>> +
>> +class QAPIGenC(QAPIGen):
>> +
>> +    def __init__(self, blurb, pydoc):
>> +        QAPIGen.__init__(self)
>> +        self._blurb = blurb.strip('\n')
>> +        self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>> +                                                  re.MULTILINE))
>>
>> -def open_output(output_dir, do_c, do_h, prefix, c_file, h_file, blurb, doc):
>> -    guard = guardname(prefix + h_file)
>> -    c_file = output_dir + prefix + c_file
>> -    h_file = output_dir + prefix + h_file
>> -    copyright = '\n * '.join(re.findall(r'^Copyright .*', doc, 
>> re.MULTILINE))
>> -    comment = mcgen('''/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>> +    def top(self, fname):
>> +        return mcgen('''
>> +/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>>
>>  /*
>>  %(blurb)s
>> @@ -1992,41 +2026,19 @@ def open_output(output_dir, do_c, do_h, prefix, 
>> c_file, h_file, blurb, doc):
>>   */
>>
>>  ''',
>> -                    blurb=blurb.strip('\n'), copyright=copyright)
>> +                     blurb=self._blurb, copyright=self._copyright)
>>
>> -    if output_dir:
>> -        try:
>> -            os.makedirs(output_dir)
>> -        except os.error as e:
>> -            if e.errno != errno.EEXIST:
>> -                raise
>>
>> -    def maybe_open(really, name, opt):
>> -        if really:
>> -            return open(name, opt)
>> -        else:
>> -            import StringIO
>> -            return StringIO.StringIO()
>> +class QAPIGenH(QAPIGenC):
>>
>> -    fdef = maybe_open(do_c, c_file, 'w')
>> -    fdecl = maybe_open(do_h, h_file, 'w')
>> +    def top(self, fname):
>> +        return QAPIGenC.top(self, fname) + guardstart(fname)
>>
>> -    fdef.write(comment)
>> -    fdecl.write(comment)
>> -    fdecl.write(mcgen('''
>> -#ifndef %(guard)s
>> -#define %(guard)s
>> +    def bottom(self, fname):
>> +        return guardend(fname)
>>
>> -''',
>> -                      guard=guard))
>>
>> -    return (fdef, fdecl)
>> -
>> -
>> -def close_output(fdef, fdecl):
>> -    fdecl.write(mcgen('''
>> -
>> -#endif
>> -'''))
>> -    fdecl.close()
>> -    fdef.close()
>> +class QAPIGenDoc(QAPIGen):
>> +    def top(self, fname):
>> +        return (QAPIGen.top(self, fname)
>> +                + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')
>> --
>> 2.13.6
>>
>
> Reviewed-by: Marc-André Lureau <address@hidden>

Thanks!