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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/02/2018 07:03 AM, Markus Armbruster 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(-)
>> 
>
> A little bit longer due to more structure, but reasonable diffstat in
> that it shows the conversion is fairly straightforward and opens the
> doors for later patches to use the new structures more effectively.
>
>>  
>>  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)
>
> I don't know if it is worth a sentence in the commit message that the
> visitor variable is renamed from 'gen' to 'vis' for less confusion with
> the new class instances 'genc' and 'genh'.

Did the rename give you pause when reviewing?

>> +++ 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')
>
> Tweaks like this means you were paying attention to still producing
> identical generated files; always a good sign.
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!