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

[Markus Armbruster]

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.