| Message ID | 20210608120723.2268181-5-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: untie 'if' conditions from C preprocessor | expand |
marcandre.lureau@redhat.com writes: > 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> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Tested-by: John Snow <jsnow@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? > + """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 :) > + 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? > + > + 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. > + > + > +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. > + > + 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. > + > + 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? > 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. > > > 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):
Hi On Mon, Jun 14, 2021 at 6:39 PM Markus Armbruster <armbru@redhat.com> wrote: > marcandre.lureau@redhat.com writes: > > > 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> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > Tested-by: John Snow <jsnow@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): > > >
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: + """An 'if' condition predicate""" + + def cgen(self) -> str: + raise NotImplementedError() + + def docgen(self) -> str: + raise NotImplementedError() + + +class IfOption(IfPredicate): + 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})" + + def __eq__(self, other: object) -> bool: + if not isinstance(other, IfOption): + return 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"{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 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]) + + 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 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):