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

[John Snow <jsnow@redhat.com>]

On 10/15/20 12:52 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The following patches are going to express conditions in some target
> language agostic way. For that, let's start building a predicate tree of
> configuration options.
> 
> The tree will be less expressive than full C macros expressions, and
> will be based only on identifier. For this reason, it will use have
> operations: 'all', 'any' and 'not'. ('and'/'or' have a connotation of
> expression operands)
> 
> For now, the documentation will use the C condition expression (as
> that's already what is exposed with defined(FOO), &&, || etc).
> Eventually, we may want to generate a more human friendly syntax later.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   docs/sphinx/qapidoc.py                 |  6 +--
>   scripts/qapi/common.py                 | 69 ++++++++++++++++++++------
>   tests/qapi-schema/doc-good.out         | 12 ++---
>   tests/qapi-schema/qapi-schema-test.out | 58 +++++++++++-----------
>   4 files changed, 91 insertions(+), 54 deletions(-)
> 
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index db9520f37f..31b47e4745 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -112,12 +112,10 @@ class QAPISchemaGenRSTVisitor(QAPISchemaVisitor):
>       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: C-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.gen_if())]
>           if not with_if:
>               return condlist
>   
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 58ddec3bc5..29e0697c27 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -194,33 +194,72 @@ def guardend(name: str) -> str:
>                    name=c_fname(name).upper())
>   
>   
> +class IfPredicate:
> +    def cgen(self) -> str:
> +        raise NotImplementedError()
> +
> +

docstrings will be helpful here for your final submission, when you get 
to it.

I guess right now the tree is IfCond -> IfPredicate -> IfAll -> IfOption.

> +class IfOption(IfPredicate):
> +    def __init__(self, option: str):
> +        self.option = option
> +
> +    def cgen(self) -> str:
> +        return self.option
> +

cgen being baked into the core implementation strikes me as odd, but I'm 
not sure where you're headed here just yet.

> +    def __repr__(self) -> str:
> +        return repr(self.option)
> +
> +    def __eq__(self, other: object) -> bool:
> +        if not isinstance(other, IfOption):
> +            return False
> +        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 __bool__(self) -> bool:
> +        return bool(self.pred_list)
> +
> +    def __repr__(self) -> str:
> +        return f"IfAll({self.pred_list})"
> +
> +    def __eq__(self, other: object) -> bool:
> +        if not isinstance(other, IfAll):
> +            return False
> +        return self.pred_list == other.pred_list
> +
> +
>   class IfCond:
>       def __init__(self, ifcond: Optional[Sequence[str]] = None):
> -        self.ifcond = ifcond or []
> +        pred_list = [IfOption(opt) for opt in ifcond or []]
> +        self.pred = IfAll(pred_list)
>   

okay; so we're converting each string into an IfOption, then creating a 
predicate (All) with all the options as children.

Note that for some things like machine-target.json that we are passing 
multiple things as the ifcond, all as one string:

machine-target.json:  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) 
|| defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }

and in this case you'll have *one* option. I assume this is probably fine?

>       def __bool__(self) -> bool:
> -        return bool(self.ifcond)
> +        return bool(self.pred)
>   
>       def __repr__(self) -> str:
> -        return repr(self.ifcond)
> +        return repr(self.pred)
>   
>       def __eq__(self, other: object) -> bool:
>           if not isinstance(other, IfCond):
>               return NotImplemented
> -        return self.ifcond == other.ifcond
> +        return self.pred == other.pred
>   
>       def gen_if(self) -> str:
> -        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) -> str:
> -        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 ""

Not your fault: reading these weirdly indented triple quote strings in 
changelogs is really difficult.

> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 419284dae2..0d27323c47 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)'])
>       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 8868ca0dca..8ba658cd38 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 union_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 union_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_TestIfUnionCmd-arg
>       member union_cmd_arg: TestIfUnion optional=False
> -    if ['defined(TEST_IF_UNION)']
> +    if IfAll(['defined(TEST_IF_UNION)'])
>   command TestIfUnionCmd q_obj_TestIfUnionCmd-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_TestIfAlternateCmd-arg
>       member alt_cmd_arg: TestIfAlternate optional=False
> -    if ['defined(TEST_IF_ALT)']
> +    if IfAll(['defined(TEST_IF_ALT)'])
>   command TestIfAlternateCmd q_obj_TestIfAlternateCmd-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_TestIfCmd-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 TestIfCmd q_obj_TestIfCmd-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 TestCmdReturnDefThree 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_TestIfEvent-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 TestIfEvent q_obj_TestIfEvent-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-FEATURES1 None
>       boxed=False
>       feature deprecated
>