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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).