qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 02/38] qapi-gen: Separate arg-parsing from generation


From: John Snow
Subject: Re: [PATCH v2 02/38] qapi-gen: Separate arg-parsing from generation
Date: Fri, 25 Sep 2020 11:37:10 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0

On 9/25/20 7:34 AM, Markus Armbruster wrote:
Cleber Rosa <crosa@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 <jsnow@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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]