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

[Markus Armbruster <armbru@redhat.com>]

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:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> The following patches are going to express schema 'if' conditions in
>> a
>> target language agnostic way. For that, let's start building a predicate
>> tree of the configuration options.
>> This intermediary steps still uses C-preprocessor expressions as
>> the predicates:
>> "if: [STR, ..]" is translated to a "IfCond -> IfAll ->
>> [IfOption, ..]" tree, which will generate "#if STR && .." C code.
>> Once the boolean operation tree nodes are introduced, the 'if'
>> expressions will be converted to replace the C syntax (no more
>> !defined(), &&, ...) and based only on option identifiers.
>> For now, the condition tree will be less expressive than a full C
>> macro
>> expression as it will only support these operations: 'all', 'any' and
>> 'not', the only ones needed so far.
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>   docs/sphinx/qapidoc.py                 |  6 +--
>>   scripts/qapi/common.py                 | 54 +++++++++++++++++++++++-
>>   scripts/qapi/schema.py                 | 42 ++++++++++++-------
>>   tests/qapi-schema/doc-good.out         | 12 +++---
>>   tests/qapi-schema/qapi-schema-test.out | 58 +++++++++++++-------------
>>   5 files changed, 116 insertions(+), 56 deletions(-)
>> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> index b737949007..a93f3f1c4d 100644
>> --- a/docs/sphinx/qapidoc.py
>> +++ b/docs/sphinx/qapidoc.py
>> @@ -112,12 +112,10 @@ def _make_section(self, title):
>>       def _nodes_for_ifcond(self, ifcond, with_if=True):
>>           """Return list of Text, literal nodes for the ifcond
>>   -        Return a list which gives text like ' (If: cond1, cond2,
>> cond3)', where
>> -        the conditions are in literal-text and the commas are not.
>> +        Return a list which gives text like ' (If: condition)'.
>>           If with_if is False, we don't return the "(If: " and ")".
>>           """
>> -        condlist = intersperse([nodes.literal('', c) for c in ifcond.ifcond],
>> -                               nodes.Text(', '))
>
> was this the last user of intersperse?
>
> mm, no, there's one more.
>
>> +        condlist = [nodes.literal('', ifcond.gen_doc())]
>>           if not with_if:
>>               return condlist
>>   diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> 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().

I don't see a need to fuzz around with module abc, either.  Let's stick
to the stupidest solution that could possibly work.

[...]