Hi
On Thu, May 13, 2021 at 12:53 AM John Snow <jsnow@redhat.com
<mailto:jsnow@redhat.com>> wrote:
On 4/29/21 9:40 AM, marcandre.lureau@redhat.com
<mailto:marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com
<mailto:marcandre.lureau@redhat.com>>
>
> Wrap the 'if' condition in a higher-level object. Not only this
allows
> more type safety but also further refactoring without too much churn.
>
Would have done it myself if I had gotten to it first. I like having a
named type for this, it matches the other properties we have.
> The following patches will change the syntax of the schema 'if'
> conditions to be predicate expressions, and will generate code for
> different target languages (C, and Rust in another series).
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com
<mailto:marcandre.lureau@redhat.com>>
> ---
> docs/sphinx/qapidoc.py | 2 +-
> scripts/qapi/commands.py | 4 +-
> scripts/qapi/events.py | 5 ++-
> scripts/qapi/gen.py | 14 +++----
> scripts/qapi/introspect.py | 26 ++++++-------
> scripts/qapi/schema.py | 78
+++++++++++++++++++++++++++-----------
> scripts/qapi/types.py | 33 ++++++++--------
> scripts/qapi/visit.py | 23 +++++------
> 8 files changed, 110 insertions(+), 75 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 87c67ab23f..b737949007 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -116,7 +116,7 @@ def _nodes_for_ifcond(self, ifcond,
with_if=True):
> the conditions are in literal-text and the commas are not.
> If with_if is False, we don't return the "(If: " and ")".
> """
> - condlist = intersperse([nodes.literal('', c) for c in
ifcond],
> + condlist = intersperse([nodes.literal('', c) for c in
ifcond.ifcond],
> nodes.Text(', '))
> if not with_if:
> return condlist
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 0e13d51054..3654825968 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -17,7 +17,6 @@
> Dict,
> List,
> Optional,
> - Sequence,
> Set,
> )
>
> @@ -31,6 +30,7 @@
> from .schema import (
> QAPISchema,
> QAPISchemaFeature,
> + QAPISchemaIfCond,
> QAPISchemaObjectType,
> QAPISchemaType,
> )
> @@ -301,7 +301,7 @@ def visit_end(self) -> None:
> def visit_command(self,
> name: str,
> info: Optional[QAPISourceInfo],
> - ifcond: Sequence[str],
> + ifcond: QAPISchemaIfCond,
> features: List[QAPISchemaFeature],
> arg_type: Optional[QAPISchemaObjectType],
> ret_type: Optional[QAPISchemaType],
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index fee8c671e7..82475e84ec 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -12,7 +12,7 @@
> See the COPYING file in the top-level directory.
> """
>
> -from typing import List, Optional, Sequence
> +from typing import List, Optional
>
> from .common import c_enum_const, c_name, mcgen
> from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
> @@ -20,6 +20,7 @@
> QAPISchema,
> QAPISchemaEnumMember,
> QAPISchemaFeature,
> + QAPISchemaIfCond,
> QAPISchemaObjectType,
> )
> from .source import QAPISourceInfo
> @@ -227,7 +228,7 @@ def visit_end(self) -> None:
> def visit_event(self,
> name: str,
> info: Optional[QAPISourceInfo],
> - ifcond: Sequence[str],
> + ifcond: QAPISchemaIfCond,
> features: List[QAPISchemaFeature],
> arg_type: Optional[QAPISchemaObjectType],
> boxed: bool) -> None:
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 1fa503bdbd..1c5b190276 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -18,7 +18,6 @@
> Dict,
> Iterator,
> Optional,
> - Sequence,
> Tuple,
> )
>
> @@ -32,6 +31,7 @@
> mcgen,
> )
> from .schema import (
> + QAPISchemaIfCond,
> QAPISchemaModule,
> QAPISchemaObjectType,
> QAPISchemaVisitor,
> @@ -85,7 +85,7 @@ def write(self, output_dir: str) -> None:
> fp.write(text)
>
>
> -def _wrap_ifcond(ifcond: Sequence[str], before: str, after: str)
-> str:
> +def _wrap_ifcond(ifcond: QAPISchemaIfCond, before: str, after:
str) -> str:
> if before == after:
> return after # suppress empty #if ... #endif
>
> @@ -95,9 +95,9 @@ def _wrap_ifcond(ifcond: Sequence[str], before:
str, after: str) -> str:
> if added[0] == '\n':
> out += '\n'
> added = added[1:]
> - out += gen_if(ifcond)
> + out += gen_if(ifcond.ifcond)
> out += added
> - out += gen_endif(ifcond)
> + out += gen_endif(ifcond.ifcond)
> return out
>
>
> @@ -127,9 +127,9 @@ def build_params(arg_type:
Optional[QAPISchemaObjectType],
> class QAPIGenCCode(QAPIGen):
> def __init__(self, fname: str):
> super().__init__(fname)
> - self._start_if: Optional[Tuple[Sequence[str], str, str]]
= None
> + self._start_if: Optional[Tuple[QAPISchemaIfCond, str,
str]] = None
>
> - def start_if(self, ifcond: Sequence[str]) -> None:
> + def start_if(self, ifcond: QAPISchemaIfCond) -> None:
> assert self._start_if is None
> self._start_if = (ifcond, self._body, self._preamble)
>
> @@ -187,7 +187,7 @@ def _bottom(self) -> str:
>
>
> @contextmanager
> -def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) ->
Iterator[None]:
> +def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) ->
Iterator[None]:
> """
> A with-statement context manager that wraps with
`start_if()` / `end_if()`.
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 9a348ca2e5..77a8c33ad4 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -15,11 +15,9 @@
> Any,
> Dict,
> Generic,
> - Iterable,
> List,
> Optional,
> Sequence,
> - Tuple,
> TypeVar,
> Union,
> )
> @@ -38,6 +36,7 @@
> QAPISchemaEntity,
> QAPISchemaEnumMember,
> QAPISchemaFeature,
> + QAPISchemaIfCond,
> QAPISchemaObjectType,
> QAPISchemaObjectTypeMember,
> QAPISchemaType,
> @@ -91,11 +90,11 @@ class Annotated(Generic[_ValueT]):
> """
> # TODO: Remove after Python 3.7 adds @dataclass:
> # pylint: disable=too-few-public-methods
> - def __init__(self, value: _ValueT, ifcond: Iterable[str],
> + def __init__(self, value: _ValueT, ifcond: QAPISchemaIfCond,
> comment: Optional[str] = None):
> self.value = value
> self.comment: Optional[str] = comment
> - self.ifcond: Tuple[str, ...] = tuple(ifcond)
> + self.ifcond = ifcond
>
>
> def _tree_to_qlit(obj: JSONValue,
> @@ -125,10 +124,10 @@ def indent(level: int) -> str:
> if obj.comment:
> ret += indent(level) + f"/* {obj.comment} */\n"
> if obj.ifcond:
> - ret += gen_if(obj.ifcond)
> + ret += gen_if(obj.ifcond.ifcond)
> ret += _tree_to_qlit(obj.value, level)
> if obj.ifcond:
> - ret += '\n' + gen_endif(obj.ifcond)
> + ret += '\n' + gen_endif(obj.ifcond.ifcond)
> return ret
>
> ret = ''
> @@ -254,7 +253,7 @@ def _gen_features(features:
Sequence[QAPISchemaFeature]
> return [Annotated(f.name <http://f.name>, f.ifcond) for
f in features]
>
> def _gen_tree(self, name: str, mtype: str, obj: Dict[str,
object],
> - ifcond: Sequence[str] = (),
> + ifcond: QAPISchemaIfCond = QAPISchemaIfCond(),
> features: Sequence[QAPISchemaFeature] = ())
-> None:
> """
> Build and append a SchemaInfo object to self._trees.
> @@ -305,7 +304,7 @@ def visit_builtin_type(self, name: str, info:
Optional[QAPISourceInfo],
> self._gen_tree(name, 'builtin', {'json-type': json_type})
>
> def visit_enum_type(self, name: str, info:
Optional[QAPISourceInfo],
> - ifcond: Sequence[str],
> + ifcond: QAPISchemaIfCond,
> features: List[QAPISchemaFeature],
> members: List[QAPISchemaEnumMember],
> prefix: Optional[str]) -> None:
> @@ -316,14 +315,14 @@ def visit_enum_type(self, name: str, info:
Optional[QAPISourceInfo],
> )
>
> def visit_array_type(self, name: str, info:
Optional[QAPISourceInfo],
> - ifcond: Sequence[str],
> + ifcond: QAPISchemaIfCond,
> element_type: QAPISchemaType) -> None:
> element = self._use_type(element_type)
> self._gen_tree('[' + element + ']', 'array',
{'element-type': element},
> ifcond)
>
> def visit_object_type_flat(self, name: str, info:
Optional[QAPISourceInfo],
> - ifcond: Sequence[str],
> + ifcond: QAPISchemaIfCond,
> features: List[QAPISchemaFeature],
> members:
List[QAPISchemaObjectTypeMember],
> variants:
Optional[QAPISchemaVariants]) -> None:
> @@ -336,7 +335,7 @@ def visit_object_type_flat(self, name: str,
info: Optional[QAPISourceInfo],
> self._gen_tree(name, 'object', obj, ifcond, features)
>
> def visit_alternate_type(self, name: str, info:
Optional[QAPISourceInfo],
> - ifcond: Sequence[str],
> + ifcond: QAPISchemaIfCond,
> features: List[QAPISchemaFeature],
> variants: QAPISchemaVariants) -> None:
> self._gen_tree(
> @@ -348,7 +347,7 @@ def visit_alternate_type(self, name: str,
info: Optional[QAPISourceInfo],
> )
>
> def visit_command(self, name: str, info:
Optional[QAPISourceInfo],
> - ifcond: Sequence[str],
> + ifcond: QAPISchemaIfCond,
> features: List[QAPISchemaFeature],
> arg_type: Optional[QAPISchemaObjectType],
> ret_type: Optional[QAPISchemaType], gen:
bool,
> @@ -367,7 +366,8 @@ def visit_command(self, name: str, info:
Optional[QAPISourceInfo],
> self._gen_tree(name, 'command', obj, ifcond, features)
>
> def visit_event(self, name: str, info:
Optional[QAPISourceInfo],
> - ifcond: Sequence[str], features:
List[QAPISchemaFeature],
> + ifcond: QAPISchemaIfCond,
> + features: List[QAPISchemaFeature],
> arg_type: Optional[QAPISchemaObjectType],
> boxed: bool) -> None:
> assert self._schema is not None
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 3a4172fb74..7d6f390fa6 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -25,6 +25,22 @@
> from .parser import QAPISchemaParser
>
>
> +class QAPISchemaIfCond:
> + def __init__(self, ifcond=None):
> + self.ifcond = ifcond or []
> +
> + def __bool__(self):
> + return bool(self.ifcond)
> +
> + def __repr__(self):
> + return repr(self.ifcond)
> +
I suggest using:
f"QAPISchemaIfCond({self.ifcond!r})"
unless future patches make that weirder. It's nice when repr() returns
something you can use to make a new, equivalent object.
eval(repr(x)) == x
I implemented it this way for compatibility with test-qapi output. But
it is simpler to actually no implement __repr__ and just change the test.