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

[John Snow <jsnow@redhat.com>]

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