On 10/15/20 12:52 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Wrap the 'if' condition in a higher-level object. Not only this is
> allows more type safety but also further refactoring without too much
> chrun. The following patches will extend the syntax of 'if' and will
> have some extra handling and types.
>
Probably a good idea. Thanks for basing it on Pt6; I'll try to push
ahead as fast as I can -- though there are some more aggressive cleanups
in error, expr, and parser that we haven't discussed on list yet much
and are quite prone to change.
Let me know if you have any comments or feedbacks regarding what you
found there!
Overall, I was pretty happy with the result. Although it seemed a bit awkward to have types everywhere in Python (it doesn't feel as consistent as with other languages, I think I need to get used to it)
Pts 2 (introspect.py) and 3 (expr.py) are recently re-sent to list, if
you have specific critique in those areas.
Done, not much comments
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> docs/sphinx/qapidoc.py | 2 +-
> scripts/qapi/commands.py | 4 +-
> scripts/qapi/common.py | 26 ++++++++---
> scripts/qapi/events.py | 4 +-
> scripts/qapi/gen.py | 9 ++--
> scripts/qapi/introspect.py | 21 ++++-----
> scripts/qapi/schema.py | 91 ++++++++++++++++++++------------------
> scripts/qapi/types.py | 11 ++---
> scripts/qapi/visit.py | 9 ++--
> 9 files changed, 102 insertions(+), 75 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 11e97839de..db9520f37f 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -116,7 +116,7 @@ class QAPISchemaGenRSTVisitor(QAPISchemaVisitor):
> 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 50978090b4..03deac5fdd 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -20,7 +20,7 @@ from typing import (
> Set,
> )
>
> -from .common import c_name, mcgen
> +from .common import IfCond, c_name, mcgen
> from .gen import (
> QAPIGenC,
> QAPIGenCCode,
> @@ -301,7 +301,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
> def visit_command(self,
> name: str,
> info: QAPISourceInfo,
> - ifcond: List[str],
> + ifcond: IfCond,
> features: List[QAPISchemaFeature],
> arg_type: Optional[QAPISchemaObjectType],
> ret_type: Optional[QAPISchemaType],
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 11b86beeab..59e6a400da 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -12,7 +12,7 @@
> # See the COPYING file in the top-level directory.
>
> import re
> -from typing import Optional, Sequence
> +from typing import Optional, Sequence, Union
>
>
> #: Magic string that gets removed along with all space to its right.
> @@ -194,18 +194,34 @@ def guardend(name: str) -> str:
> name=c_fname(name).upper())
>
>
> -def gen_if(ifcond: Sequence[str]) -> str:
> +class IfCond:
> + def __init__(self, ifcond: Optional[Sequence[str]] = None):
> + self.ifcond = ifcond or []
> +
> + def __bool__(self) -> bool:
> + return bool(self.ifcond)
> +
> + def __repr__(self) -> str:
> + return repr(self.ifcond)
> +
> + def __eq__(self, other: object) -> bool:
> + if not isinstance(other, IfCond):
> + return NotImplemented
> + return self.ifcond == other.ifcond
> +
Haven't looked ahead yet, forgive me if this is a bad idea:
worth adding an __iter__ method here so that callers don't have to call
"for x in ifcond.ifcond" ?
Maybe you refactor such that this is becomes pointless.
Yes, the new _expression_ tree and generation code is quite different now. Not really worth it as an intermediary refactoring step.
Also; should we create an Ifcond object in schema.py instead in common,
as it's a generic representation of the #if conditionals, less tied to
the C generation?
Good point, I'll update.