On 9/25/20 7:34 AM, Markus Armbruster wrote:
Cleber Rosa <cr...@redhat.com> writes:
On Tue, Sep 22, 2020 at 05:00:25PM -0400, John Snow wrote:
This is a minor re-work of the entrypoint script. It isolates a
generate() method from the actual command-line mechanism.
Signed-off-by: John Snow <js...@redhat.com>
---
scripts/qapi-gen.py | 87 ++++++++++++++++++++++++++++++++-------------
1 file changed, 63 insertions(+), 24 deletions(-)
diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
index 4b03f7d53b..59becba3e1 100644
--- a/scripts/qapi-gen.py
+++ b/scripts/qapi-gen.py
@@ -1,9 +1,13 @@
#!/usr/bin/env python3
-# QAPI generator
-#
+
# This work is licensed under the terms of the GNU GPL, version 2 or later.
# See the COPYING file in the top-level directory.
+"""
+QAPI Generator
+
+This script is the main entry point for generating C code from the QAPI schema.
+"""
import argparse
import re
@@ -11,21 +15,65 @@
from qapi.commands import gen_commands
from qapi.doc import gen_doc
+from qapi.error import QAPIError
from qapi.events import gen_events
from qapi.introspect import gen_introspect
-from qapi.schema import QAPIError, QAPISchema
+from qapi.schema import QAPISchema
from qapi.types import gen_types
from qapi.visit import gen_visit
-def main(argv):
+DEFAULT_OUTPUT_DIR = ''
+DEFAULT_PREFIX = ''
I did not understand the purpose of these. If they're used only as
the default value for the command line option parsing, I'd suggest
dropping them.
+
+
+def generate(schema_file: str,
+ output_dir: str,
+ prefix: str,
+ unmask: bool = False,
+ builtins: bool = False) -> None:
+ """
+ generate uses a given schema to produce C code in the target directory.
+
+ :param schema_file: The primary QAPI schema file.
+ :param output_dir: The output directory to store generated code.
+ :param prefix: Optional C-code prefix for symbol names.
+ :param unmask: Expose non-ABI names through introspection?
+ :param builtins: Generate code for built-in types?
+
+ :raise QAPIError: On failures.
+ """
+ match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
+ if match and match.end() != len(prefix):
Nice catch with the extra check here. Maybe worth mentioning and/or
splitting the change?
Please do not sneak additional checking into patches advertized as pure
refactoring. It makes me look for more sneakery with a microscope.
This re.match() cannot possibly fail. Three cases:
* First character is funny
The regexp matches the empty string. There's a reason the regexp ends
with '?'.
* Non-first character is funny
The regexp matches the non-funny prefix.
* No character is funny
The regexp matches the complete string.
Checking impossible conditions as if they were possible is confusing.
Please drop the additional check.
We can talk about checking this impossible condition with
assert(match)
if you believe it makes the code easier to understand (it does not
improve its behavior).
My use of strict_optional=False is what prevents this from exhibiting as
an error in mypy. An assert will help convince mypy that 'match' cannot
possibly be 'None'.
eh, well. I will fix this when I remove strict_optional, so I will just
remove this additional check for now to avoid adding another patch to
this series.
--js
