Hi
On Mon, Jun 14, 2021 at 6:39 PM Markus Armbruster <arm...@redhat.com> wrote:
> marcandre.lur...@redhat.com writes:
>
> > 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>
> > Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
> > Tested-by: John Snow <js...@redhat.com>
> > ---
> > docs/sphinx/qapidoc.py | 6 +--
> > scripts/qapi/common.py | 53 ++++++++++++++++++++++-
> > scripts/qapi/schema.py | 17 ++++++--
> > tests/qapi-schema/doc-good.out | 12 +++---
> > tests/qapi-schema/qapi-schema-test.out | 58 +++++++++++++-------------
> > tests/qapi-schema/test-qapi.py | 2 +-
> > 6 files changed, 103 insertions(+), 45 deletions(-)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index b737949007..0f87fb16ce 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(', '))
> > + condlist = [nodes.literal('', ifcond.docgen())]
> > if not with_if:
> > return condlist
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index c305aaf2f1..01e3203545 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -12,7 +12,7 @@
> > # See the COPYING file in the top-level directory.
> >
> > import re
> > -from typing import Match, Optional
> > +from typing import Match, Optional, Sequence
> >
> >
> > #: Magic string that gets removed along with all space to its right.
> > @@ -214,3 +214,54 @@ def must_match(pattern: str, string: str) ->
> Match[str]:
> > match = re.match(pattern, string)
> > assert match is not None
> > return match
> > +
> > +
> > +class IfPredicate:
>
> This is the abstract base class of the two (soon four) forms 'if'.
> qapi-code-gen.txt calls them "conditionals", and the code so far uses
> names like @ifcond. I'd prefer not to throw "predicate" into the
> cauldron. IfCond? IfConditional?
>
>
ok
> > + """An 'if' condition predicate"""
> > +
> > + def cgen(self) -> str:
> > + raise NotImplementedError()
> > +
> > + def docgen(self) -> str:
> > + raise NotImplementedError()
> > +
> > +
> > +class IfOption(IfPredicate):
>
> The name IfOption tells me nothing.
>
> At this point in the series, the IfOption are arbitrary C preprocessor
> expressions.
>
> At the end of the series, they are operands of the C preprocessor's
> unary operator defined, i.e. a C macro name.
>
> Once I know that, IfOption kind of makes sense. Hmm. IfConfigOption?
> IfIdentifier? IfLeaf? I'm not quite sure which one I dislike the least
> :)
>
Ok IfConfigOption.
>
> > + 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 f"{type(self).__name__}({self.option!r})"
>
> Intended use?
>
%s in test-qapi
> > +
> > + def __eq__(self, other: object) -> bool:
> > + if not isinstance(other, IfOption):
> > + return NotImplemented
> > + return self.option == other.option
>
> Why raise on type mismatch? Feels rather un-pythonic to me.
>
removed
> > +
> > +
> > +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])
>
> Fragile. See my review of PATCH 3.
>
>
ok
> +
> > + def docgen(self) -> str:
> > + return " and ".join([p.docgen() for p in self.pred_list])
> > +
> > + def __bool__(self) -> bool:
> > + return bool(self.pred_list)
>
> Not as confusing as QAPISchemaIfCond.__bool__() as long as it's kept
> well away from None. Still, I'm not sure I like it.
>
>
removed
> +
> > + def __repr__(self) -> str:
> > + return f"{type(self).__name__}({self.pred_list!r})"
> > +
> > + def __eq__(self, other: object) -> bool:
> > + if not isinstance(other, IfAll):
> > + return NotImplemented
> > + return self.pred_list == other.pred_list
>
> Same as above.
>
> Why are these classes in common.py?
>
moved to schema.py
>
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index aa4715c519..f52caaeecc 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -19,7 +19,12 @@
> > import re
> > from typing import Optional
> >
> > -from .common import POINTER_SUFFIX, c_name
> > +from .common import (
> > + POINTER_SUFFIX,
> > + IfAll,
> > + IfOption,
> > + c_name,
> > +)
> > from .error import QAPIError, QAPISemError, QAPISourceError
> > from .expr import check_exprs
> > from .parser import QAPISchemaParser
> > @@ -28,18 +33,22 @@
> > class QAPISchemaIfCond:
> > def __init__(self, ifcond=None):
> > self.ifcond = ifcond or []
> > + self.pred = IfAll([IfOption(opt) for opt in self.ifcond])
>
> In the common case of just one element, we get a no-op IfAll wrapped
> around it. Okay.
>
> self.ifcond goes away in PATCH 7. Okay.
>
> > +
> > + def docgen(self):
> > + return self.pred.docgen()
> >
> > def cgen(self):
> > - return ' && '.join(self.ifcond)
> > + return self.pred.cgen()
> >
> > # Returns true if the condition is not void
> > def __bool__(self):
> > - return bool(self.ifcond)
> > + return bool(self.pred)
> >
> > def __eq__(self, other):
> > if not isinstance(other, QAPISchemaIfCond):
> > return NotImplemented
> > - return self.ifcond == other.ifcond
> > + return self.pred == other.pred
>
> Not much left in this class, and it's going to get even thinner.
>
Yes, see v7.
> >
> >
> > class QAPISchemaEntity:
> > diff --git a/tests/qapi-schema/doc-good.out
> b/tests/qapi-schema/doc-good.out
> > index 8f54ceff2e..db1d23c6bf 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([IfOption('defined(IFONE)')])
> > member two
> > - if ['defined(IFCOND)']
> > + if IfAll([IfOption('defined(IFCOND)')])
> > feature enum-feat
> > object Base
> > member base1: Enum optional=False
> > object Variant1
> > member var1: str optional=False
> > - if ['defined(IFSTR)']
> > + if IfAll([IfOption('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([IfOption('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([IfOption('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([IfOption('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..e4e0fb173a 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([IfOption('defined(TEST_IF_STRUCT_BAR)')])
> > + if IfAll([IfOption('defined(TEST_IF_STRUCT)')])
> > enum TestIfEnum
> > member foo
> > member bar
> > - if ['defined(TEST_IF_ENUM_BAR)']
> > - if ['defined(TEST_IF_ENUM)']
> > + if IfAll([IfOption('defined(TEST_IF_ENUM_BAR)')])
> > + if IfAll([IfOption('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([IfOption('defined(TEST_IF_UNION_BAR)')])
> > + if IfAll([IfOption('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([IfOption('defined(TEST_IF_UNION_BAR)')])
> > + if IfAll([IfOption('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([IfOption('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([IfOption('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([IfOption('defined(TEST_IF_ALT_BAR)')])
> > + if IfAll([IfOption('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([IfOption('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([IfOption('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([IfOption('defined(TEST_IF_CMD_BAR)')])
> > + if IfAll([IfOption('defined(TEST_IF_CMD)'),
> IfOption('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([IfOption('defined(TEST_IF_CMD)'),
> IfOption('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([IfOption('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([IfOption('defined(TEST_IF_EVT_BAR)')])
> > + if IfAll([IfOption('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([IfOption('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([IfOption('defined(TEST_IF_FEATURE_1)')])
> > object CondFeatureStruct2
> > member foo: int optional=False
> > feature feature1
> > - if ['defined(TEST_IF_FEATURE_1)']
> > + if IfAll([IfOption('defined(TEST_IF_FEATURE_1)')])
> > feature feature2
> > - if ['defined(TEST_IF_FEATURE_2)']
> > + if IfAll([IfOption('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([IfOption('defined(TEST_IF_COND_1)'),
> IfOption('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([IfOption('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([IfOption('defined(TEST_IF_FEATURE_1)')])
> > feature feature2
> > - if ['defined(TEST_IF_FEATURE_2)']
> > + if IfAll([IfOption('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([IfOption('defined(TEST_IF_COND_1)'),
> IfOption('defined(TEST_IF_COND_2)')])
> > event TEST_EVENT_FEATURES0 FeatureStruct1
> > boxed=False
> > event TEST_EVENT_FEATURES1 None
> > diff --git a/tests/qapi-schema/test-qapi.py
> b/tests/qapi-schema/test-qapi.py
> > index 2ec328b22e..631e255fba 100755
> > --- a/tests/qapi-schema/test-qapi.py
> > +++ b/tests/qapi-schema/test-qapi.py
> > @@ -95,7 +95,7 @@ def _print_variants(variants):
> > @staticmethod
> > def _print_if(ifcond, indent=4):
> > if ifcond:
> > - print('%sif %s' % (' ' * indent, ifcond.ifcond))
> > + print('%sif %s' % (' ' * indent, ifcond.pred))
> >
> > @classmethod
> > def _print_features(cls, features, indent=4):
>
>
>
--
Marc-André Lureau
