Hi
On Thu, May 13, 2021 at 1:39 AM John Snow <js...@redhat.com> wrote:
> On 4/29/21 9:40 AM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lur...@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.lur...@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?
>
>
As discussed, we'll see that when we get to it, I don't remember.
> + 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.
>
ok
> > + @abstractmethod
> > + def docgen(self) -> str:
> > + pass
> > + > +
> > +class IfOption(IfPredicate):
>
> missing a docstring here, but I suppose I haven't written a style guide
> on how to write those yet.
>
> I assume IfOption is one single element?
>
e.g. X && Y && Z is
>
> All(
> Option(X),
> Option(Y),
> Option(Z),
> )
>
> Right?
>
>
Correct
> > + def __init__(self, option: str):
> > + self.option = option
> > +
> > + def cgen(self) -> str:
> > + return self.option
> > +
> > + def docgen(self) -> str:
> > + return self.option
> > +
> > + def __repr__(self) -> str:
> > + return repr(self.option)
> > +
>
> type(self).__name__ + f"({self.option!r})"
>
> or just hard-code the IfOption in there
>
ok
> > + def __eq__(self, other: object) -> bool:
> > + if not isinstance(other, IfOption):
> > + return False
>
> maybe NotImplemented?
>
> (Admitting I don't know the FULL consequences of doing it either way,
> just prompting you for an explanation.)
>
Apparently NotImplemented is the best thing to do, as it will fallback in
other ways.
It doesn't really matter it seems, let's use NotImplemented.
> > + return self.option == other.option
> > +
> > +
> > +class IfAll(IfPredicate):
> > + def __init__(self, pred_list: Sequence[IfPredicate]):
> > + self.pred_list = pred_list
> > +
> > + def cgen(self) -> str:
> > + return " && ".join([p.cgen() for p in self.pred_list])
> > +
> > + def docgen(self) -> str:
> > + return " and ".join([p.docgen() for p in self.pred_list])
> > +
> > + def __bool__(self) -> bool:
> > + return bool(self.pred_list)
> > +
> > + def __repr__(self) -> str:
> > + return f"IfAll({self.pred_list})"
> > +
>
> {self.pred_list!r} to get the recursive repr.
>
ok
> You can consider adding a __str__ method to add a kind of short-hand
> that skips the class names and stuff and keeps the output less chatty.
>
>
ok
> + def __eq__(self, other: object) -> bool:
> > + if not isinstance(other, IfAll):
> > + return False
>
> NotImplemented again?
>
> > + return self.pred_list == other.pred_list
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 8e6d0a5296..366a53ab64 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -19,7 +19,13 @@
> > import re
> > from typing import Optional
> >
> > -from .common import POINTER_SUFFIX, c_name, mcgen
> > +from .common import (
> > + POINTER_SUFFIX,
> > + IfAll,
> > + IfOption,
> > + c_name,
> > + mcgen,
> > +)
> > from .error import QAPISemError, QAPISourceError
> > from .expr import check_exprs
> > from .parser import QAPISchemaParser
> > @@ -27,34 +33,38 @@
> >
> > class QAPISchemaIfCond:
> > def __init__(self, ifcond=None):
> > - self.ifcond = ifcond or []
> > + pred_list = [IfOption(opt) for opt in ifcond or []]
> > + self.pred = IfAll(pred_list)
> > +
> > + def gen_doc(self):
> > + if self.pred:
> > + return self.pred.docgen()
> > + return ""
> >
>
> pred should always be IfAll as of here, can we remove the conditional?
> .join() will deform to the empty string when it's given an empty sequence
>
>
ok
> def gen_if(self):
> > - ret = ''
> > - for ifc in self.ifcond:
> > - ret += mcgen('''
> > + if self.pred:
> > + return mcgen('''
> > #if %(cond)s
> > -''', cond=ifc)
> > - return ret
> > +''', cond=self.pred.cgen())
> > + return ""
> >
> > def gen_endif(self):
> > - ret = ''
> > - for ifc in reversed(self.ifcond):
> > - ret += mcgen('''
> > -#endif /* %(cond)s */
> > -''', cond=ifc)
> > - return ret
> > + if self.pred:
> > + return mcgen('''
> > +#endif // %(cond)s
> > +''', cond=self.pred.cgen())
> > + return ""
> >
> > def __bool__(self):
> > - return bool(self.ifcond)
> > + return bool(self.pred)
> >
> > def __repr__(self):
> > - return repr(self.ifcond)
> > + return repr(self.pred)
> >
> > def __eq__(self, other):
> > if not isinstance(other, QAPISchemaIfCond):
> > return NotImplemented
> > - return self.ifcond == other.ifcond
> > + return self.pred == other.pred
> >
> >
> > class QAPISchemaEntity:
> > diff --git a/tests/qapi-schema/doc-good.out
> b/tests/qapi-schema/doc-good.out
> > index 8f54ceff2e..6bf996f539 100644
> > --- a/tests/qapi-schema/doc-good.out
> > +++ b/tests/qapi-schema/doc-good.out
> > @@ -12,15 +12,15 @@ enum QType
> > module doc-good.json
> > enum Enum
> > member one
> > - if ['defined(IFONE)']
> > + if IfAll(['defined(IFONE)'])
>
> I suppose the test output changing, no longer representing a single
> language makes sense.
>
> > member two
> > - if ['defined(IFCOND)']
> > + if IfAll(['defined(IFCOND)'])
> > feature enum-feat
> > object Base
> > member base1: Enum optional=False
> > object Variant1
> > member var1: str optional=False
> > - if ['defined(IFSTR)']
> > + if IfAll(['defined(IFSTR)'])
> > feature member-feat
> > feature variant1-feat
> > object Variant2
> > @@ -29,7 +29,7 @@ object Object
> > tag base1
> > case one: Variant1
> > case two: Variant2
> > - if ['IFTWO']
> > + if IfAll(['IFTWO'])
> > feature union-feat1
> > object q_obj_Variant1-wrapper
> > member data: Variant1 optional=False
> > @@ -38,13 +38,13 @@ object q_obj_Variant2-wrapper
> > enum SugaredUnionKind
> > member one
> > member two
> > - if ['IFTWO']
> > + if IfAll(['IFTWO'])
> > object SugaredUnion
> > member type: SugaredUnionKind optional=False
> > tag type
> > case one: q_obj_Variant1-wrapper
> > case two: q_obj_Variant2-wrapper
> > - if ['IFTWO']
> > + if IfAll(['IFTWO'])
> > feature union-feat2
> > alternate Alternate
> > tag type
> > diff --git a/tests/qapi-schema/qapi-schema-test.out
> b/tests/qapi-schema/qapi-schema-test.out
> > index e0b8a5f0b6..c2d303aa18 100644
> > --- a/tests/qapi-schema/qapi-schema-test.out
> > +++ b/tests/qapi-schema/qapi-schema-test.out
> > @@ -298,65 +298,65 @@ command __org.qemu_x-command
> q_obj___org.qemu_x-command-arg -> __org.qemu_x-Unio
> > object TestIfStruct
> > member foo: int optional=False
> > member bar: int optional=False
> > - if ['defined(TEST_IF_STRUCT_BAR)']
> > - if ['defined(TEST_IF_STRUCT)']
> > + if IfAll(['defined(TEST_IF_STRUCT_BAR)'])
> > + if IfAll(['defined(TEST_IF_STRUCT)'])
> > enum TestIfEnum
> > member foo
> > member bar
> > - if ['defined(TEST_IF_ENUM_BAR)']
> > - if ['defined(TEST_IF_ENUM)']
> > + if IfAll(['defined(TEST_IF_ENUM_BAR)'])
> > + if IfAll(['defined(TEST_IF_ENUM)'])
> > object q_obj_TestStruct-wrapper
> > member data: TestStruct optional=False
> > enum TestIfUnionKind
> > member foo
> > member bar
> > - if ['defined(TEST_IF_UNION_BAR)']
> > - if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
> > + if IfAll(['defined(TEST_IF_UNION_BAR)'])
> > + if IfAll(['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)'])
> > object TestIfUnion
> > member type: TestIfUnionKind optional=False
> > tag type
> > case foo: q_obj_TestStruct-wrapper
> > case bar: q_obj_str-wrapper
> > - if ['defined(TEST_IF_UNION_BAR)']
> > - if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
> > + if IfAll(['defined(TEST_IF_UNION_BAR)'])
> > + if IfAll(['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)'])
> > object q_obj_test-if-union-cmd-arg
> > member union-cmd-arg: TestIfUnion optional=False
> > - if ['defined(TEST_IF_UNION)']
> > + if IfAll(['defined(TEST_IF_UNION)'])
> > command test-if-union-cmd q_obj_test-if-union-cmd-arg -> None
> > gen=True success_response=True boxed=False oob=False
> preconfig=False
> > - if ['defined(TEST_IF_UNION)']
> > + if IfAll(['defined(TEST_IF_UNION)'])
> > alternate TestIfAlternate
> > tag type
> > case foo: int
> > case bar: TestStruct
> > - if ['defined(TEST_IF_ALT_BAR)']
> > - if ['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)']
> > + if IfAll(['defined(TEST_IF_ALT_BAR)'])
> > + if IfAll(['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)'])
> > object q_obj_test-if-alternate-cmd-arg
> > member alt-cmd-arg: TestIfAlternate optional=False
> > - if ['defined(TEST_IF_ALT)']
> > + if IfAll(['defined(TEST_IF_ALT)'])
> > command test-if-alternate-cmd q_obj_test-if-alternate-cmd-arg -> None
> > gen=True success_response=True boxed=False oob=False
> preconfig=False
> > - if ['defined(TEST_IF_ALT)']
> > + if IfAll(['defined(TEST_IF_ALT)'])
> > object q_obj_test-if-cmd-arg
> > member foo: TestIfStruct optional=False
> > member bar: TestIfEnum optional=False
> > - if ['defined(TEST_IF_CMD_BAR)']
> > - if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
> > + if IfAll(['defined(TEST_IF_CMD_BAR)'])
> > + if IfAll(['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'])
> > command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree
> > gen=True success_response=True boxed=False oob=False
> preconfig=False
> > - if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
> > + if IfAll(['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'])
> > command test-cmd-return-def-three None -> UserDefThree
> > gen=True success_response=True boxed=False oob=False
> preconfig=False
> > array TestIfEnumList TestIfEnum
> > - if ['defined(TEST_IF_ENUM)']
> > + if IfAll(['defined(TEST_IF_ENUM)'])
> > object q_obj_TEST_IF_EVENT-arg
> > member foo: TestIfStruct optional=False
> > member bar: TestIfEnumList optional=False
> > - if ['defined(TEST_IF_EVT_BAR)']
> > - if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
> > + if IfAll(['defined(TEST_IF_EVT_BAR)'])
> > + if IfAll(['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)'])
> > event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
> > boxed=False
> > - if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
> > + if IfAll(['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)'])
> > object FeatureStruct0
> > member foo: int optional=False
> > object FeatureStruct1
> > @@ -379,17 +379,17 @@ object FeatureStruct4
> > object CondFeatureStruct1
> > member foo: int optional=False
> > feature feature1
> > - if ['defined(TEST_IF_FEATURE_1)']
> > + if IfAll(['defined(TEST_IF_FEATURE_1)'])
> > object CondFeatureStruct2
> > member foo: int optional=False
> > feature feature1
> > - if ['defined(TEST_IF_FEATURE_1)']
> > + if IfAll(['defined(TEST_IF_FEATURE_1)'])
> > feature feature2
> > - if ['defined(TEST_IF_FEATURE_2)']
> > + if IfAll(['defined(TEST_IF_FEATURE_2)'])
> > object CondFeatureStruct3
> > member foo: int optional=False
> > feature feature1
> > - if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)']
> > + if IfAll(['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'])
> > enum FeatureEnum1
> > member eins
> > member zwei
> > @@ -429,17 +429,17 @@ command test-command-features3 None -> None
> > command test-command-cond-features1 None -> None
> > gen=True success_response=True boxed=False oob=False
> preconfig=False
> > feature feature1
> > - if ['defined(TEST_IF_FEATURE_1)']
> > + if IfAll(['defined(TEST_IF_FEATURE_1)'])
> > command test-command-cond-features2 None -> None
> > gen=True success_response=True boxed=False oob=False
> preconfig=False
> > feature feature1
> > - if ['defined(TEST_IF_FEATURE_1)']
> > + if IfAll(['defined(TEST_IF_FEATURE_1)'])
> > feature feature2
> > - if ['defined(TEST_IF_FEATURE_2)']
> > + if IfAll(['defined(TEST_IF_FEATURE_2)'])
> > command test-command-cond-features3 None -> None
> > gen=True success_response=True boxed=False oob=False
> preconfig=False
> > feature feature1
> > - if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)']
> > + if IfAll(['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'])
> > event TEST_EVENT_FEATURES0 FeatureStruct1
> > boxed=False
> > event TEST_EVENT_FEATURES1 None
> >
>
> Tested-by: John Snow <js...@redhat.com>
>
> Seems fine again, minor style qualms about Python I am not that direly
> opinionated on. More worried about the integration of cgen and docgen,
> though I see why with a predicate tree it becomes *extremely* convenient
> to integrate them right into the class.
>
> Is there a way to have our cake and eat it, too ... ?
>
> (Maybe a visitor of some kind is the right way to go?)
>
That's also what I thought (it was in my commit message comments).
Repeating myself, I'd defer this, there is no urge to make the code more
complex yet. It can easily be done in a following iteration.
