From: John Snow <jsnow@redhat.com>
To: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: qemu-devel <qemu-devel@nongnu.org>,
"Armbruster, Markus" <armbru@redhat.com>
Subject: Re: [PATCH v3 1/9] qapi: replace List[str] by QAPISchemaIfCond
Date: Mon, 17 May 2021 12:30:06 -0400 [thread overview]
Message-ID: <676860f9-efa7-6e17-f356-c1ca512ab9fc@redhat.com> (raw)
In-Reply-To: <CAMxuvaxZDZF_=CY6h41QktAqAwHZvGwqvpTb3hfTCgxE73H3eg@mail.gmail.com>
On 5/17/21 7:17 AM, Marc-André Lureau wrote:
> 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.
>
I like having the __repr__ methods personally, but if it's for test
output purposes, probably best to change the test, yeah.
You can use __str__ instead and have the tests print str(node) or
{node!s} without worrying about __repr__ hygiene.
> > + def __eq__(self, other):
> > + if not isinstance(other, QAPISchemaIfCond):
> > + return NotImplemented
> > + return self.ifcond == other.ifcond
> > +
> > +
>
> Missing annotations, but ... yeah, schema.py isn't typed yet so I will
> handle it myself in pt6. No biggie.
>
>
> There used to be annotations all over in earlier versions, but I rebased
> and had to drop most of them.
>
Yep, don't worry about it for now. I'll get to it.
next prev parent reply other threads:[~2021-05-17 16:50 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-29 13:40 [PATCH v3 0/9] qapi: untie 'if' conditions from C preprocessor marcandre.lureau
2021-04-29 13:40 ` [PATCH v3 1/9] qapi: replace List[str] by QAPISchemaIfCond marcandre.lureau
2021-05-12 20:53 ` John Snow
2021-05-17 11:17 ` Marc-André Lureau
2021-05-17 16:30 ` John Snow [this message]
2021-04-29 13:40 ` [PATCH v3 2/9] qapi: move gen_if/gen_endif to QAPISchemaIfCond marcandre.lureau
2021-05-11 16:39 ` Stefan Hajnoczi
2021-05-12 12:36 ` Marc-André Lureau
2021-05-12 18:53 ` John Snow
2021-05-17 11:18 ` Marc-André Lureau
2021-05-12 21:01 ` John Snow
2021-05-21 14:35 ` Markus Armbruster
2021-04-29 13:40 ` [PATCH v3 3/9] qapi: start building an 'if' predicate tree marcandre.lureau
2021-05-12 21:39 ` John Snow
2021-05-17 11:18 ` Marc-André Lureau
2021-05-17 16:34 ` John Snow
2021-05-21 14:48 ` Markus Armbruster
2021-05-21 16:18 ` John Snow
2021-06-08 13:57 ` Markus Armbruster
2021-04-29 13:40 ` [PATCH v3 4/9] qapi: introduce IfPredicateList and IfAny marcandre.lureau
2021-05-12 23:26 ` John Snow
2021-05-17 11:18 ` Marc-André Lureau
2021-05-17 16:35 ` John Snow
2021-04-29 13:40 ` [PATCH v3 5/9] qapi: add IfNot marcandre.lureau
2021-05-12 23:32 ` John Snow
2021-04-29 13:40 ` [PATCH v3 6/9] qapi: normalize 'if' condition to IfPredicate tree marcandre.lureau
2021-05-12 23:47 ` John Snow
2021-05-17 11:18 ` Marc-André Lureau
2021-05-17 16:41 ` John Snow
2021-04-29 13:40 ` [PATCH v3 7/9] qapi: convert 'if' C-expressions to the new syntax tree marcandre.lureau
2021-05-12 23:51 ` John Snow
2021-05-17 11:20 ` Marc-André Lureau
2021-04-29 13:40 ` [PATCH v3 8/9] qapi: make 'if' condition strings simple identifiers marcandre.lureau
2021-05-12 23:56 ` John Snow
2021-04-29 13:40 ` [PATCH v3 9/9] docs: update the documentation about schema configuration marcandre.lureau
2021-05-13 0:01 ` John Snow
2021-05-11 16:52 ` [PATCH v3 0/9] qapi: untie 'if' conditions from C preprocessor Stefan Hajnoczi
2021-05-12 12:39 ` Marc-André Lureau
2021-05-12 17:43 ` Markus Armbruster
2021-05-12 18:58 ` John Snow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=676860f9-efa7-6e17-f356-c1ca512ab9fc@redhat.com \
--to=jsnow@redhat.com \
--cc=armbru@redhat.com \
--cc=marcandre.lureau@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.