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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

True.  But see below.

>> * 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 <address@hidden>
>> ---
>
>>   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 <address@hidden>
>> +#  Marc-André Lureau <address@hidden>
>> +#
>> +# 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.

When Anthony and Mike created QAPI, they licensed it v2-only.  I have no
idea why.

By now, precious little code from that version is left.  Almost all
current code has been written by myself, you, Marc-André, and Kevin.

Nevertheless, we're boxed into a v2-only corner.  I resent that.

I should've slapped "contributions after <today> are GPLv2+" on the
whole thing the day I accepted to maintain it, if not earlier (right
when I started rewriting the parts that cried for it).

In my opinion, we should reject contributions unless licensed GPLv2+ as
a matter of policy, with exceptions granted on a case-by-case basis.

>> +++ 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").

Perhaps a bit of shell wizardry can increase your confidence.

Before this patch:

1. Split into classes and functions (crudely!):

    $ csplit scripts/qapi/common.py '/^\(class\|def\) /' '{*}'

2. Rename the parts:

    $ for i in xx*; do n=`sed -n '1s/^[a-z]* \([A-Za-z0-9_]*\).*/\1/p' $i`; [ 
"$n" ] && mv $i xx-$n; done

3. Stash them:

    $ mkdir o
    $ $ mv xx* o

After this patch:

1. Split:

    $ csplit <(cat 
scripts/qapi/{common,source,error,parser,expr,schema,gen}.py) '/^\(class\|def\) 
/' '{*}'

2. Rename:

    $ for i in xx*; do n=`sed -n '1s/^[a-z]* \([A-Za-z0-9_]*\).*/\1/p' $i`; [ 
"$n" ] && mv $i xx-$n; done

3. Stash & diff:

    $ mkdir n
    $ mv xx* n
    $ diff -rup o n

Output of diff appended for your reading pleasure.

> Reviewed-by: Eric Blake <address@hidden>

Thanks!


diff -rup o/xx-QAPIDoc n/xx-QAPIDoc
--- o/xx-QAPIDoc        2019-10-02 17:02:35.984552694 +0200
+++ n/xx-QAPIDoc        2019-10-02 17:06:17.930607336 +0200
@@ -273,5 +273,31 @@ class QAPIDoc(object):
                 self.info,
                 "the following documented members are not in "
                 "the declaration: %s" % ", ".join(bogus))
+#
+# Check (context-free) QAPI schema expression structure
+#
+# Copyright IBM, Corp. 2011
+# Copyright (c) 2013-2019 Red Hat Inc.
+#
+# Authors:
+#  Anthony Liguori <address@hidden>
+#  Markus Armbruster <address@hidden>
+#  Eric Blake <address@hidden>
+#  Marc-André Lureau <address@hidden>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+import re
+from collections import OrderedDict
+from qapi.common import c_name
+from qapi.error import QAPISemError
+
+
+# Names must be letters, numbers, -, and _.  They must start with letter,
+# except for downstream extensions which must start with __RFQDN_.
+# Dots are only valid in the downstream extension prefix.
+valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
+                        '[a-zA-Z][a-zA-Z0-9_-]*$')
 
 
diff -rup o/xx-QAPIGen n/xx-QAPIGen
--- o/xx-QAPIGen        2019-10-02 17:02:35.987552655 +0200
+++ n/xx-QAPIGen        2019-10-02 17:06:17.932607309 +0200
@@ -43,4 +43,3 @@ class QAPIGen(object):
         f.close()
 
 
-@contextmanager
diff -rup o/xx-QAPIGenH n/xx-QAPIGenH
--- o/xx-QAPIGenH       2019-10-02 17:02:35.987552655 +0200
+++ n/xx-QAPIGenH       2019-10-02 17:06:17.933607296 +0200
@@ -7,3 +7,4 @@ class QAPIGenH(QAPIGenC):
         return guardend(self.fname)
 
 
+@contextmanager
diff -rup o/xx-QAPISchema n/xx-QAPISchema
--- o/xx-QAPISchema     2019-10-02 17:02:35.986552668 +0200
+++ n/xx-QAPISchema     2019-10-02 17:06:17.932607309 +0200
@@ -297,9 +297,26 @@ class QAPISchema(object):
                     visitor.visit_module(module)
                 entity.visit(visitor)
         visitor.visit_end()
-
-
 #
-# Code generation helpers
+# QAPI code generation
+#
+# Copyright (c) 2018-2019 Red Hat Inc.
+#
+# Authors:
+#  Markus Armbruster <address@hidden>
+#  Marc-André Lureau <address@hidden>
 #
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+
+import errno
+import os
+import re
+import sys
+from contextlib import contextmanager
+
+from qapi.common import *
+from qapi.schema import QAPISchemaVisitor
+
 
diff -rup o/xx-QAPISchemaParser n/xx-QAPISchemaParser
--- o/xx-QAPISchemaParser       2019-10-02 17:02:35.984552694 +0200
+++ n/xx-QAPISchemaParser       2019-10-02 17:06:17.930607336 +0200
@@ -268,14 +268,3 @@ class QAPISchemaParser(object):
         raise QAPIParseError(self, "documentation comment must end with '##'")
 
 
-#
-# Check (context-free) schema expression structure
-#
-
-# Names must be letters, numbers, -, and _.  They must start with letter,
-# except for downstream extensions which must start with __RFQDN_.
-# Dots are only valid in the downstream extension prefix.
-valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
-                        '[a-zA-Z][a-zA-Z0-9_-]*$')
-
-
diff -rup o/xx-QAPISemError n/xx-QAPISemError
--- o/xx-QAPISemError   2019-10-02 17:02:35.984552694 +0200
+++ n/xx-QAPISemError   2019-10-02 17:06:17.930607336 +0200
@@ -1,5 +1,27 @@
 class QAPISemError(QAPIError):
     def __init__(self, info, msg):
         QAPIError.__init__(self, info, None, msg)
+#
+# QAPI schema parser
+#
+# Copyright IBM, Corp. 2011
+# Copyright (c) 2013-2019 Red Hat Inc.
+#
+# Authors:
+#  Anthony Liguori <address@hidden>
+#  Markus Armbruster <address@hidden>
+#  Marc-André Lureau <address@hidden>
+#  Kevin Wolf <address@hidden>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+import os
+import re
+import sys
+from collections import OrderedDict
+
+from qapi.error import QAPIParseError, QAPISemError
+from qapi.source import QAPISourceInfo
 
 
diff -rup o/xx-QAPISourceInfo n/xx-QAPISourceInfo
--- o/xx-QAPISourceInfo 2019-10-02 17:02:35.984552694 +0200
+++ n/xx-QAPISourceInfo 2019-10-02 17:06:17.930607336 +0200
@@ -40,5 +40,16 @@ class QAPISourceInfo(object):
 
     def __str__(self):
         return self.include_path() + self.in_defn() + self.loc()
+#
+# QAPI error classes
+#
+# Copyright (c) 2017-2019 Red Hat Inc.
+#
+# Authors:
+#  Markus Armbruster <address@hidden>
+#  Marc-André Lureau <address@hidden>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
 
 
diff -rup o/xx-build_params n/xx-build_params
--- o/xx-build_params   2019-10-02 17:02:35.987552655 +0200
+++ n/xx-build_params   2019-10-02 17:06:17.930607336 +0200
@@ -17,9 +17,17 @@ def build_params(arg_type, boxed, extra=
     if extra:
         ret += sep + extra
     return ret if ret else 'void'
-
-
 #
-# Accumulate and write output
+# QAPI frontend source file info
+#
+# Copyright (c) 2019 Red Hat Inc.
 #
+# Authors:
+#  Markus Armbruster <address@hidden>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+import copy
+
 
Only in o: xx-camel_case
diff -rup o/xx-check_exprs n/xx-check_exprs
--- o/xx-check_exprs    2019-10-02 17:02:35.985552681 +0200
+++ n/xx-check_exprs    2019-10-02 17:06:17.931607323 +0200
@@ -80,10 +80,28 @@ def check_exprs(exprs):
         check_flags(expr, info)
 
     return exprs
-
-
 #
-# Schema compiler frontend
-# TODO catching name collisions in generated code would be nice
+# QAPI schema internal representation
+#
+# Copyright (c) 2015-2019 Red Hat Inc.
+#
+# Authors:
+#  Markus Armbruster <address@hidden>
+#  Eric Blake <address@hidden>
+#  Marc-André Lureau <address@hidden>
 #
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+# TODO catching name collisions in generated code would be nice
+
+import os
+import re
+from collections import OrderedDict
+
+from qapi.common import c_name, pointer_suffix
+from qapi.error import QAPIError, QAPIParseError, QAPISemError
+from qapi.expr import check_exprs
+from qapi.parser import QAPISchemaParser
+
 
diff -rup o/xx00 n/xx00
--- o/xx00      2019-10-02 17:02:35.983552708 +0200
+++ n/xx00      2019-10-02 17:06:17.929607349 +0200
@@ -11,19 +11,10 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
-from __future__ import print_function
-from contextlib import contextmanager
-import copy
-import errno
-import os
 import re
 import string
-import sys
-from collections import OrderedDict
-
-
-#
-# Parsing the schema into expressions
-#
 
 
+# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
+# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
+# ENUM24_Name -> ENUM24_NAME