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>
---
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(', '))
+ 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:
+ pass
+
+ @abstractmethod
+ def docgen(self) -> str:
+ pass
+
+
+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 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 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})"
+
+ def __eq__(self, other: object) -> bool:
+ if not isinstance(other, IfAll):
+ return False
+ 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 ""
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)'])
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
--
2.29.0
On 4/29/21 9:40 AM, marcandre.lureau@redhat.com wrote:
> 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>
> ---
> 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?
> + 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.
> + @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?
> + 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
> + 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.)
> + 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.
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.
> + 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
> 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 <jsnow@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?)
--js
Hi
On Thu, May 13, 2021 at 1:39 AM John Snow <jsnow@redhat.com> wrote:
> On 4/29/21 9:40 AM, marcandre.lureau@redhat.com wrote:
> > 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>
> > ---
> > 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 <jsnow@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.
On 5/17/21 7:18 AM, Marc-André Lureau wrote: > That's also what I thought (it was in my commit message comments). I did read them, I promise :) > 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. I think I can accept this, as long as we don't create a lot of work for ourselves splitting it back out later. I think it'll be up to Markus, ultimately. --js
Beware, I'm skimming, not really reviewing.
John Snow <jsnow@redhat.com> writes:
> On 4/29/21 9:40 AM, marcandre.lureau@redhat.com wrote:
>> 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>
>> ---
>> 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?
>
>> + 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.
We're not coding a library for use by thousands of people. If we did,
then complicating the code to guard against misuse could be a win. But
we don't.
schema.py is full of methods that pass. Maybe some of them need to be
overriden by any conceivable subtype. Who cares? The subtypes either
work or they don't. I prefer
def frobnicate:
pass
to
def frobnicate:
raise NotImplementedError
One, pass is easier on the eyes. Two, a subtype's .frobnicate() can
blindly call super().frobnicate().
I don't see a need to fuzz around with module abc, either. Let's stick
to the stupidest solution that could possibly work.
[...]
On 5/21/21 10:48 AM, Markus Armbruster wrote:
> Beware, I'm skimming, not really reviewing.
>
> John Snow <jsnow@redhat.com> writes:
>
>> On 4/29/21 9:40 AM, marcandre.lureau@redhat.com wrote:
>>> 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>
>>> ---
>>> 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?
>>
>>> + 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.
>
> We're not coding a library for use by thousands of people. If we did,
> then complicating the code to guard against misuse could be a win. But
> we don't.
>
> schema.py is full of methods that pass. Maybe some of them need to be
> overriden by any conceivable subtype. Who cares? The subtypes either
> work or they don't. I prefer
>
> def frobnicate:
> pass
>
> to
>
> def frobnicate:
> raise NotImplementedError
>
> One, pass is easier on the eyes. Two, a subtype's .frobnicate() can
> blindly call super().frobnicate().
>
"pass" here operates as a syntactic sugar for "return None" which has
implications on the return type. It's short, but wrong.
raise NotImplementedError means something different semantically.
To me, pass is *NOT* easier on the eyes, it is misleading. It is
idiomatic to use NotImplementedError if it is not acceptable for a
default implementation to return None.
I understand perfectly well the desire to keep things simple, but what
is actually "simple" depends on the expectations of the programmer. I
err towards idiomatic Python.
> I don't see a need to fuzz around with module abc, either. Let's stick
> to the stupidest solution that could possibly work.
>
> [...]
>
John Snow <jsnow@redhat.com> writes:
> On 5/21/21 10:48 AM, Markus Armbruster wrote:
>> Beware, I'm skimming, not really reviewing.
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 4/29/21 9:40 AM, marcandre.lureau@redhat.com wrote:
[...]
>>>> 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?
>>>
>>>> + 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.
>>
>> We're not coding a library for use by thousands of people. If we did,
>> then complicating the code to guard against misuse could be a win. But
>> we don't.
>>
>> schema.py is full of methods that pass. Maybe some of them need to be
>> overriden by any conceivable subtype. Who cares? The subtypes either
>> work or they don't. I prefer
>>
>> def frobnicate:
>> pass
>>
>> to
>>
>> def frobnicate:
>> raise NotImplementedError
>>
>> One, pass is easier on the eyes. Two, a subtype's .frobnicate() can
>> blindly call super().frobnicate().
>>
>
> "pass" here operates as a syntactic sugar for "return None" which has
> implications on the return type. It's short, but wrong.
Nitpicking...
pass is not specified as syntactic sugar:
7.4. The pass statement
pass_stmt ::= "pass"
pass is a null operation — when it is executed, nothing happens. It is useful as a placeholder when a statement is required syntactically, but no code needs to be executed, for example:
def f(arg): pass # a function that does nothing (yet)
class C: pass # a class with no methods (yet)
What really happens in
def foo(): pass
is what *always* happens when control reaches the end of the function
body: the function returns None.
Further evidence:
>>> def baz():
... pass
... return 42
...
>>> baz()
42
> raise NotImplementedError means something different semantically.
>
> To me, pass is *NOT* easier on the eyes, it is misleading. It is
> idiomatic to use NotImplementedError if it is not acceptable for a
> default implementation to return None.
>
> I understand perfectly well the desire to keep things simple, but what
> is actually "simple" depends on the expectations of the programmer. I
> err towards idiomatic Python.
What's your take on "Two, a subtype's .frobnicate() can blindly call
super().frobnicate()"?
[...]
© 2016 - 2026 Red Hat, Inc.