Hi
On Thu, Aug 5, 2021 at 5:42 PM Markus Armbruster <arm...@redhat.com> wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > Replace the simple list sugar form with a recursive structure that will
> > accept other operators in the following commits (all, any or not).
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > scripts/qapi/common.py | 23 +++++--
> > scripts/qapi/expr.py | 52 ++++++++++------
> > scripts/qapi/schema.py | 2 +-
> > tests/qapi-schema/bad-if-all.err | 2 +
> > tests/qapi-schema/bad-if-all.json | 3 +
> > tests/qapi-schema/bad-if-all.out | 0
> > tests/qapi-schema/bad-if-empty-list.json | 2 +-
> > tests/qapi-schema/bad-if-key.err | 3 +
> > tests/qapi-schema/bad-if-key.json | 3 +
> > tests/qapi-schema/bad-if-key.out | 0
> > tests/qapi-schema/bad-if-keys.err | 2 +
> > tests/qapi-schema/bad-if-keys.json | 3 +
> > tests/qapi-schema/bad-if-keys.out | 0
> > tests/qapi-schema/bad-if-list.json | 2 +-
> > tests/qapi-schema/bad-if.err | 2 +-
> > tests/qapi-schema/bad-if.json | 2 +-
> > tests/qapi-schema/doc-good.json | 3 +-
> > tests/qapi-schema/doc-good.out | 13 ++--
> > tests/qapi-schema/doc-good.txt | 6 ++
> > tests/qapi-schema/enum-if-invalid.err | 3 +-
> > tests/qapi-schema/features-if-invalid.err | 2 +-
> > tests/qapi-schema/meson.build | 3 +
> > tests/qapi-schema/qapi-schema-test.json | 25 ++++----
> > tests/qapi-schema/qapi-schema-test.out | 62 +++++++++----------
> > .../qapi-schema/struct-member-if-invalid.err | 2 +-
> > .../qapi-schema/union-branch-if-invalid.json | 2 +-
> > 26 files changed, 138 insertions(+), 84 deletions(-)
> > create mode 100644 tests/qapi-schema/bad-if-all.err
> > create mode 100644 tests/qapi-schema/bad-if-all.json
> > create mode 100644 tests/qapi-schema/bad-if-all.out
> > create mode 100644 tests/qapi-schema/bad-if-key.err
> > create mode 100644 tests/qapi-schema/bad-if-key.json
> > create mode 100644 tests/qapi-schema/bad-if-key.out
> > create mode 100644 tests/qapi-schema/bad-if-keys.err
> > create mode 100644 tests/qapi-schema/bad-if-keys.json
> > create mode 100644 tests/qapi-schema/bad-if-keys.out
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 5181a0f167..51463510c9 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -13,7 +13,8 @@
> >
> > import re
> > from typing import (
> > - List,
> > + Any,
> > + Dict,
> > Match,
> > Optional,
> > Union,
> > @@ -199,16 +200,28 @@ def guardend(name: str) -> str:
> > name=c_fname(name).upper())
> >
> >
> > -def cgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> > +def docgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
>
> Looks like you forgot to un-swap cgen_ifcond() and docgen_ifcond(). Can
> do in my tree.
>
Oops, I did it though, I wonder where it went..!
> > if not ifcond:
> > return ''
> > - return '(' + ') && ('.join(ifcond) + ')'
> > + if isinstance(ifcond, str):
> > + return ifcond
> >
> > + oper, operands = next(iter(ifcond.items()))
> > + oper = {'all': ' and '}[oper]
> > + operands = [docgen_ifcond(o) for o in operands]
> > + return '(' + oper.join(operands) + ')'
> >
> > -def docgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> > +
> > +def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
> > if not ifcond:
> > return ''
> > - return ' and '.join(ifcond)
> > + if isinstance(ifcond, str):
> > + return ifcond
> > +
> > + oper, operands = next(iter(ifcond.items()))
> > + oper = {'all': '&&'}[oper]
> > + operands = [cgen_ifcond(o) for o in operands]
> > + return '(' + (') ' + oper + ' (').join(operands) + ')'
>
> I suggested a more legible version in review of v6. Not worth a respin
> by itself.
>
> Note to self: try to get rid of redundant parenthesis here.
>
> Note to self: cgen_ifcond() and docgen_ifcond() are almost identical.
> Refactor?
>
> >
> >
> > def gen_if(cond: str) -> str:
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index cf98923fa6..b5187bfbca 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -259,14 +259,12 @@ def check_flags(expr: _JSONObject, info:
> QAPISourceInfo) -> None:
> >
> > def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) ->
> None:
> > """
> > - Normalize and validate the ``if`` member of an object.
> > + Validate the ``if`` member of an object.
> >
> > - The ``if`` member may be either a ``str`` or a ``List[str]``.
> > - A ``str`` value will be normalized to ``List[str]``.
> > + The ``if`` member may be either a ``str`` or a dict.
> >
> > :forms:
> > - :sugared: ``Union[str, List[str]]``
> > - :canonical: ``List[str]``
> > + :canonical: ``Union[str, dict]``
>
> John hasn't answered my question whether :forms: makes sensw without
> :sugared:. If it doesn't, I can drop it in my tree.
>
> >
> > :param expr: The expression containing the ``if`` member to
> validate.
> > :param info: QAPI schema source file information.
> > @@ -275,31 +273,45 @@ def check_if(expr: _JSONObject, info:
> QAPISourceInfo, source: str) -> None:
> > :raise QAPISemError:
> > When the "if" member fails validation, or when there are no
> > non-empty conditions.
> > - :return: None, ``expr`` is normalized in-place as needed.
> > + :return: None
> > """
> > ifcond = expr.get('if')
> > if ifcond is None:
> > return
> >
> > - if isinstance(ifcond, list):
> > - if not ifcond:
> > - raise QAPISemError(
> > - info, "'if' condition [] of %s is useless" % source)
> > - else:
> > - # Normalize to a list
> > - ifcond = expr['if'] = [ifcond]
> > + def _check_if(cond: Union[str, object]) -> None:
> > + if isinstance(cond, str):
> > + if not cond.strip():
> > + raise QAPISemError(
> > + info,
> > + "'if' condition '%s' of %s makes no sense"
> > + % (cond, source))
> > + return
> >
> > - for elt in ifcond:
> > - if not isinstance(elt, str):
> > + if not isinstance(cond, dict):
> > raise QAPISemError(
> > info,
> > - "'if' condition of %s must be a string or a list of
> strings"
> > - % source)
> > - if not elt.strip():
> > + "'if' condition of %s must be a string or a dict" %
> source)
> > + if len(cond) != 1:
> > raise QAPISemError(
> > info,
> > - "'if' condition '%s' of %s makes no sense"
> > - % (elt, source))
> > + "'if' condition dict of %s must have one key: "
> > + "'all'" % source)
> > + check_keys(cond, info, "'if' condition", [],
> > + ["all"])
> > +
> > + oper, operands = next(iter(cond.items()))
> > + if not operands:
> > + raise QAPISemError(
> > + info, "'if' condition [] of %s is useless" % source)
> > +
> > + if oper in ("all") and not isinstance(operands, list):
> > + raise QAPISemError(
> > + info, "'%s' condition of %s must be a list" % (oper,
> source))
> > + for operand in operands:
> > + _check_if(operand)
> > +
> > + _check_if(ifcond)
>
> Mind if I squash in the move of the helper function to the beginning?
>
>
No problem
>
> >
> > def normalize_members(members: object) -> None:
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index ff9c4f0a17..627735a431 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -32,7 +32,7 @@
> >
> > class QAPISchemaIfCond:
> > def __init__(self, ifcond=None):
> > - self.ifcond = ifcond or []
> > + self.ifcond = ifcond or {}
>
> This is slightly subtle.
>
> QAPISchemaIfCond.ifcond can look like one of
>
> * {'all': [COND, ...]}
>
> * {'any': [COND, ...]} (only after PATCH 07)
>
> * {'not': COND} (only after PATCH 08)
>
> * STRING
>
> * {}
>
> This is just like the AST, except "absent" is now {} instead of None.
> It can occur only at the root.
>
> cgen_ifcond() and docgen_ifcond() are recursive, which means they
> happily accept {} anywhere, and generate crap.
>
> I believe the code works anyway, because it only ever creates {} in
> QAPISchemaIfCond.__init__(), i.e. at the root.
>
> Non-local correctness argument. I'd like to try my hand at tweaking the
> code so it's more obviously correct. Not now.
>
> >
> > def cgen(self):
> > return cgen_ifcond(self.ifcond)
> [Tests skipped for now...]
>
>
>
thanks
--
Marc-André Lureau
