[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: |
Markus Armbruster |
Subject: |
Re: [PATCH v2 02/38] qapi-gen: Separate arg-parsing from generation |
Date: |
Mon, 28 Sep 2020 13:45:06 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
John Snow <jsnow@redhat.com> writes:
> 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>
[...]
>>>> +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'.
Adding assertions to help mypy along is okay.
> 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.
Makes sense to me.
- [PATCH v2 01/38] [DO-NOT-MERGE] qapi: add debugging tools, (continued)
[PATCH v2 06/38] qapi: delint using flake8, John Snow, 2020/09/22
[PATCH v2 07/38] qapi: add pylintrc, John Snow, 2020/09/22
[PATCH v2 09/38] qapi/common.py: Add indent manager, John Snow, 2020/09/22