Re: [PATCH 6/7] qapi: Split up scripts/qapi/common.py

From: Eric Blake <eblake@redhat.com>
To: Markus Armbruster <armbru@redhat.com>, qemu-devel@nongnu.org
Cc: marcandre.lureau@redhat.com, mdroth@linux.vnet.ibm.com
Subject: Re: [PATCH 6/7] qapi: Split up scripts/qapi/common.py
Date: Tue, 1 Oct 2019 16:19:42 -0500	[thread overview]
Message-ID: <f195452f-43f8-a1be-653b-e4d8d2d888e3@redhat.com> (raw)
In-Reply-To: <20191001191514.11208-7-armbru@redhat.com>

On 10/1/19 2:15 PM, Markus Armbruster wrote:
> The QAPI code generator clocks in at some 3100 SLOC in 8 source files.
> Almost 60% of the code is in qapi/common.py.  Split it into more
> focused modules:
> 
> * Move QAPISchemaPragma and QAPISourceInfo to qapi/source.py.
> 
> * Move QAPIError and its sub-classes to qapi/error.py.
> 
> * Move QAPISchemaParser and QAPIDoc to parser.py.  Use the opportunity
>    to put QAPISchemaParser first.
> 
> * Move check_expr() & friends to qapi/expr.py.  Use the opportunity to
>    put the code into a more sensible order.

Code motion can be easier to review when it is 1:1 (using 'diff -u <(sed 
-n '/^-//p' patch) <(sed -n '/^\+//p'patch)', which is quite small if 
code moved wholesale).  Reordering things breaks that property.

> 
> * Move QAPISchema & friends to qapi/schema.py
> 
> * Move QAPIGen and its sub-classes, ifcontext,
>    QAPISchemaModularCVisitor, and QAPISchemaModularCVisitor to qapi/gen.py
> 
> A number of helper functions remain in qapi/common.py.  I considered
> moving the code generator helpers to qapi/gen.py, but decided not to.
> Perhaps we should rewrite them as methods of QAPIGen some day.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

>   15 files changed, 2411 insertions(+), 2329 deletions(-)

Sheesh. This one's big.  I'm half-tempted to ask you to split it 
further.  But here goes my review anyway...

>   create mode 100644 scripts/qapi/error.py
>   create mode 100644 scripts/qapi/expr.py
>   create mode 100644 scripts/qapi/gen.py
>   create mode 100644 scripts/qapi/parser.py
>   create mode 100644 scripts/qapi/schema.py
>   create mode 100644 scripts/qapi/source.py
> 
> diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> index 3d98ca2e0c..f93f3c7c23 100755
> --- a/scripts/qapi-gen.py

> +++ b/scripts/qapi/error.py
> @@ -0,0 +1,42 @@
> +#
> +# QAPI error classes
> +#
> +# Copyright (c) 2017-2019 Red Hat Inc.
> +#
> +# Authors:
> +#  Markus Armbruster <armbru@redhat.com>
> +#  Marc-André Lureau <marcandre.lureau@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.
> +# See the COPYING file in the top-level directory.

It's a shame the generator got stuck at GPLv2-only.  I don't know if 
that's worth cleaning up as part of refactoring, but if so, it would be 
best as a separate patch from the code motion.

> +++ b/scripts/qapi/gen.py

> +++ b/scripts/qapi/parser.py

> +++ b/scripts/qapi/schema.py

> +++ b/scripts/qapi/source.py
I didn't see any obvious accidental changes in all that motion (although 
given the size, the review was more cursory of the form "does it look 
like an entire chunk of code moved from one file to another per the 
commit message" than "read every line").

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org