On Tue, Aug 3, 2021 at 9:05 AM Markus Armbruster wrote: > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau > > > > 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 > > --- > > scripts/qapi/common.py | 23 +++++-- > > scripts/qapi/expr.py | 52 ++++++++++------ > > scripts/qapi/schema.py | 2 +- > > tests/qapi-schema/bad-if-empty-list.json | 2 +- > > tests/qapi-schema/bad-if-list.json | 2 +- > > tests/qapi-schema/bad-if.err | 3 +- > > 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/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 +- > > 15 files changed, 119 insertions(+), 83 deletions(-) > > > > 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: > > Uh, why do you swap cgen_ifcond() and docgen_ifcond()? Accident? > > > 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) + ')' > > What a nice review speedbump you buried here... > > The whole block boils down to the much less exciting > > operands = [docgen_ifcond(o) for o in ifcond['all']] > return '(' + ' and '.join(operands) + ')' > > Peeking ahead, I understand that you did it this way here so you can > extend it trivially there. Matter of taste; what counts is the final > result and minimizing reviewer WTFs/minute along the way. > > Since the WTFs/minute is a done deed now, what remains is the final > result, which I expect to review shortly. But please try a bit harder > to be boring next time ;) > > > > > -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 > > This is what gets rid of the redundant parenthesises in the common case > "single condition string". > > > + > > + oper, operands = next(iter(ifcond.items())) > > + oper = {'all': '&&'}[oper] > > + operands = [cgen_ifcond(o) for o in operands] > > + return '(' + (') ' + oper + ' (').join(operands) + ')' > > This line is hard to read. Easier, I think: > > oper = {'all': ' && '}[oper] > operands = ['(' + cgen_ifcond(o) + ')' for o in operands] > return oper.join(operands) > > Neither your version nor mine gets rid of the redundant parenthesises in > the (uncommon) case "complex condition expression". > tests/test-qapi-introspect.c still has > > #if (defined(TEST_IF_COND_1)) && (defined(TEST_IF_COND_2)) > QLIT_QSTR("feature1"), > #endif /* (defined(TEST_IF_COND_1)) && (defined(TEST_IF_COND_2)) */ > > Mildly annoying. I'm willing to leave this for later. > > Code smell: cgen_ifcond() and docgen_ifcond() are almost identical. Can > also be left for later. > > > > > > > def gen_if(cond: str) -> str: > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > > index 496f7e0333..3ee66c5f62 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]`` > > Does this :forms: thing make sense without any :sugared:? John, you > added (invented?) it in commit a48653638fa, but no explanation made it > into the tree. > > This is just a "field list" ... it's just markup that renders like a bulleted definition list kind of thing. The :field list: syntax is useful only so far as we use it consistently; does it make sense without at least 2 entries? it CAN, if by analogy with the other docstrings. It's just a visual consistency thing, it doesn't have any special meaning. i.e. unlike the other field lists (param, return, raise) it has no special recognition by the Sphinx python domain. I won't push very hard for having it be kept either way, though Union[str, dict] is kind of a cop-out and doesn't actually convey the concrete form, which was the intent of adding these in the first place. --js