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?
> + """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):
