Re: [PATCH v3 3/9] qapi: start building an 'if' predicate tree

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 5/21/21 10:48 AM, Markus Armbruster wrote:
>> Beware, I'm skimming, not really reviewing.
>> 
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 4/29/21 9:40 AM, marcandre.lureau@redhat.com wrote:

[...]

>>>> index b7f475a160..59a7ee2f32 100644
>>>> --- a/scripts/qapi/common.py
>>>> +++ b/scripts/qapi/common.py
>>>> @@ -11,8 +11,9 @@
>>>>    # This work is licensed under the terms of the GNU GPL, version 2.
>>>>    # See the COPYING file in the top-level directory.
>>>>    +from abc import ABC, abstractmethod
>>>>    import re
>>>> -from typing import Optional
>>>> +from typing import Optional, Sequence
>>>>      
>>>>    #: Magic string that gets removed along with all space to its right.
>>>> @@ -192,3 +193,54 @@ def guardend(name: str) -> str:
>>>>    #endif /* %(name)s */
>>>>    ''',
>>>>                     name=c_fname(name).upper())
>>>> +
>>>> +
>>>> +class IfPredicate(ABC):
>>>> +    @abstractmethod
>>>> +    def cgen(self) -> str:
>>>
>>> Like the review to patch 2, I'm not sure we want to bake cgen stuff
>>> directly into this class. Are you going to have cgen() and rustgen()
>>> methods all here?
>>>
>>>> +        pass
>>>> +
>>>
>>> I think you want raise NotImplementedError to specify a function that
>>> the inheriting class MUST implement. Otherwise, there's little value
>>> to allow a child class to call super() on a method that doesn't have a
>>> default implementation.
>>>
>>> You *could* drop the (ABC) and the @abstractmethod decorators if you do so.
>>>
>>> Matters of taste and style.
>> 
>> We're not coding a library for use by thousands of people.  If we did,
>> then complicating the code to guard against misuse could be a win.  But
>> we don't.
>> 
>> schema.py is full of methods that pass.  Maybe some of them need to be
>> overriden by any conceivable subtype.  Who cares?  The subtypes either
>> work or they don't.  I prefer
>> 
>>      def frobnicate:
>>          pass
>> 
>> to
>> 
>>      def frobnicate:
>>          raise NotImplementedError
>> 
>> One, pass is easier on the eyes.  Two, a subtype's .frobnicate() can
>> blindly call super().frobnicate().
>> 
>
> "pass" here operates as a syntactic sugar for "return None" which has 
> implications on the return type. It's short, but wrong.

Nitpicking...

pass is not specified as syntactic sugar:

    7.4. The pass statement

    pass_stmt ::=  "pass"

    pass is a null operation — when it is executed, nothing happens. It is 
useful as a placeholder when a statement is required syntactically, but no code 
needs to be executed, for example:

    def f(arg): pass    # a function that does nothing (yet)

    class C: pass       # a class with no methods (yet)

What really happens in

    def foo(): pass

is what *always* happens when control reaches the end of the function
body: the function returns None.

Further evidence:

    >>> def baz():
    ...     pass
    ...     return 42
    ... 
    >>> baz()
    42

> raise NotImplementedError means something different semantically.
>
> To me, pass is *NOT* easier on the eyes, it is misleading. It is 
> idiomatic to use NotImplementedError if it is not acceptable for a 
> default implementation to return None.
>
> I understand perfectly well the desire to keep things simple, but what 
> is actually "simple" depends on the expectations of the programmer. I 
> err towards idiomatic Python.

What's your take on "Two, a subtype's .frobnicate() can blindly call
super().frobnicate()"?

[...]