Re: [PATCH v6 07/11] qapi: replace if condition list with dict {'all': [...]}

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6199 bytes --]

On Tue, Aug 3, 2021 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@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.lureau@redhat.com>
> > ---
> >  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

[-- Attachment #2: Type: text/html, Size: 7983 bytes --]