From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ED03BC4338F for ; Wed, 4 Aug 2021 08:29:10 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4148E60EBB for ; Wed, 4 Aug 2021 08:29:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 4148E60EBB Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:46132 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mBCGz-0000n0-D6 for qemu-devel@archiver.kernel.org; Wed, 04 Aug 2021 04:29:09 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:33670) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mBCBe-0004pN-Iy for qemu-devel@nongnu.org; Wed, 04 Aug 2021 04:23:38 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:48175) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mBCBZ-0003DV-Uz for qemu-devel@nongnu.org; Wed, 04 Aug 2021 04:23:38 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1628065413; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=x4gjTzKYjYFyGSqZCdvJ63hjXf7t36NZpZgqSfw40mc=; b=Ib7qPurt57q/NjTMcCfNoRP0+N5qBvm/ImbpQSOwVFDtabs7YZsNxLjmtchleI0AaOjjiN QG4lPp4wjk8V+Sjba3T+jVciC2wJCQazpF6A3av1PY/Vr2NgfzZ3gDsP3AOmbO957cKqrv SDLdwdKiDqkvcziz8pMN+/WTbfFNym8= Received: from mail-pj1-f69.google.com (mail-pj1-f69.google.com [209.85.216.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-600-lADqO-LiOsmTMttOuBIAow-1; Wed, 04 Aug 2021 04:23:31 -0400 X-MC-Unique: lADqO-LiOsmTMttOuBIAow-1 Received: by mail-pj1-f69.google.com with SMTP id n4-20020a17090ac684b0290177656cfbc7so1815653pjt.7 for ; Wed, 04 Aug 2021 01:23:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=x4gjTzKYjYFyGSqZCdvJ63hjXf7t36NZpZgqSfw40mc=; b=iHvagr8kX+yvGLnloUf+EkOnd0vAY+RomAvN7IusIpl1e37yUPY4W2aXj3eU6AdSoh PL0wQ/a1rb/KYnI+fdyjCzAO1unepZQ8xzVP89qt09tkge6FWkKKxQ481D6tJBl+TXFF +RVM1fFxrC1EOoS6IUGHUdcYSfH4N5xkWIRjaWrpvpYYH4qKf30cJ/CLo3OSLqhRMzDW FFTOVKVM9/fTg4xMCVhEBzMyvKOoDxFYrH4ebPN4BABSJrnmowaV2Aa0ABx9xmnUzObP E4D9PB945aWDxxwVdWuh6NBwBciwQeMZ0qhzSG9mcCoPoiamAjeVBpoB1UufMBv0ByiG w5Ag== X-Gm-Message-State: AOAM530Cfe25hpIBfC4WIKye4DcCe3lMZeK+9up68+9fFdQ2Ei2Kj1Ds UjzNmq0RuUCk1GcWaYDjuxYAWGBZjRRR6v9Uf4idesR1YvwXPUqNs1Z932vJieCylsRfgBXE0Db wf11tRIXrrhq1d27ogPxNck7ftpVdQVQ= X-Received: by 2002:a63:1155:: with SMTP id 21mr700380pgr.346.1628065410222; Wed, 04 Aug 2021 01:23:30 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzlqDt5Pn9g/mJdfYLHsHC9Hf/0wDUeiUe2D0boJ5bqw36r3CukFTlF6HJw/3JTXKGJ3Ew3bvMPi+tY+EG6uWU= X-Received: by 2002:a63:1155:: with SMTP id 21mr700363pgr.346.1628065410026; Wed, 04 Aug 2021 01:23:30 -0700 (PDT) MIME-Version: 1.0 References: <20210618102507.3761128-1-marcandre.lureau@redhat.com> <20210618102507.3761128-8-marcandre.lureau@redhat.com> <87lf5ismpj.fsf@dusky.pond.sub.org> In-Reply-To: <87lf5ismpj.fsf@dusky.pond.sub.org> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Wed, 4 Aug 2021 12:23:18 +0400 Message-ID: Subject: Re: [PATCH v6 07/11] qapi: replace if condition list with dict {'all': [...]} To: Markus Armbruster Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=mlureau@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="0000000000006f91ab05c8b78435" Received-SPF: pass client-ip=170.10.133.124; envelope-from=mlureau@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -34 X-Spam_score: -3.5 X-Spam_bar: --- X-Spam_report: (-3.5 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.699, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Eric Blake , John Snow , qemu-devel , Stefan Hajnoczi Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --0000000000006f91ab05c8b78435 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi On Tue, Aug 3, 2021 at 5:05 PM Markus Armbruster wrote: > marcandre.lureau@redhat.com writes: > > > From: Marc-Andr=C3=A9 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=C3=A9 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=3Dc_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? > Yes, fixed > > if not ifcond: > > return '' > > - return '(' + ') && ('.join(ifcond) + ')' > > + if isinstance(ifcond, str): > > + return ifcond > > > > + oper, operands =3D next(iter(ifcond.items())) > > + oper =3D {'all': ' and '}[oper] > > + operands =3D [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 =3D [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 ;) > Ah sorry, I didn't realize while splitting the patches > > > > -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 =3D next(iter(ifcond.items())) > > + oper =3D {'all': '&&'}[oper] > > + operands =3D [cgen_ifcond(o) for o in operands] > > + return '(' + (') ' + oper + ' (').join(operands) + ')' > > This line is hard to read. Easier, I think: > > oper =3D {'all': ' && '}[oper] > operands =3D ['(' + 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. > I see > Code smell: cgen_ifcond() and docgen_ifcond() are almost identical. Can > also be left for later. > ok > > > > > > 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. > > > > > :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 > > """ > > Looks like there's a bit more going on than the commit message made me > expect. > > > ifcond =3D 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 =3D expr['if'] =3D [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) !=3D 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 =3D 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) > > Putting the function's helper in the middle of the function reminds me > of Mark Twain's "The Awful German Language": > > "The trunks being now ready, he DE- after kissing his mother and > sisters, and once more pressing to his bosom his adored Gretchen, > who, dressed in simple white muslin, with a single tuberose in the > ample folds of her rich brown hair, had tottered feebly down the > stairs, still pale from the terror and excitement of the past > evening, but longing to lay her poor aching head yet once again upon > the breast of him whom she loved more dearly than life itself, > PARTED." > > I find it hard to read. > > Matter of taste, I guess. I'll let you fix it up to your preference as follow up if you don't mind. > > > > > > def normalize_members(members: object) -> None: > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index 30d6a01ad1..d2fbdbe583 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > @@ -32,7 +32,7 @@ > > > > class QAPISchemaIfCond: > > def __init__(self, ifcond=3DNone): > > - self.ifcond =3D ifcond or [] > > + self.ifcond =3D ifcond or {} > > > > def cgen(self): > > return cgen_ifcond(self.ifcond) > > diff --git a/tests/qapi-schema/bad-if-empty-list.json > b/tests/qapi-schema/bad-if-empty-list.json > > index 94f2eb8670..b62b5671df 100644 > > --- a/tests/qapi-schema/bad-if-empty-list.json > > +++ b/tests/qapi-schema/bad-if-empty-list.json > > @@ -1,3 +1,3 @@ > > # check empty 'if' list > > { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' }, > > - 'if': [] } > > + 'if': { 'all': [] } } > > diff --git a/tests/qapi-schema/bad-if-list.json > b/tests/qapi-schema/bad-if-list.json > > index ea3d95bb6b..1fefef16a7 100644 > > --- a/tests/qapi-schema/bad-if-list.json > > +++ b/tests/qapi-schema/bad-if-list.json > > @@ -1,3 +1,3 @@ > > # check invalid 'if' content > > { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' }, > > - 'if': ['foo', ' '] } > > + 'if': { 'all': ['foo', ' '] } } > > diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.er= r > > index f83dee65da..8278c49368 100644 > > --- a/tests/qapi-schema/bad-if.err > > +++ b/tests/qapi-schema/bad-if.err > > @@ -1,2 +1,3 @@ > > bad-if.json: In struct 'TestIfStruct': > > -bad-if.json:2: 'if' condition of struct must be a string or a list of > strings > > +bad-if.json:2: 'if' condition has unknown key 'value' > > +Valid keys are 'all'. > > "keys are" is awkward when there's just one. Okay since there soon will > be more. > > right Test case bad-if.json is meant to cover "value of key 'if' has an > invalid JSON type". Before the patch, str and list are valid, and the > test uses (invalid) dict. Afterwards, str and dict are, and the test > still uses (now valid) dict. In other words, it now tests something > else entirely. > > I think this test should be updated to something like > > 'if': [ 'defined(TEST_IF_STRUCT)' ] > > and a new test added to cover invalid dict key. > ok, and I added some tests to cover new error paths. > > diff --git a/tests/qapi-schema/doc-good.json > b/tests/qapi-schema/doc-good.json > > index 423ea23e07..25b1053e8a 100644 > > --- a/tests/qapi-schema/doc-good.json > > +++ b/tests/qapi-schema/doc-good.json > > @@ -70,7 +70,8 @@ > > # @base1: > > # the first member > > ## > > -{ 'struct': 'Base', 'data': { 'base1': 'Enum' } } > > +{ 'struct': 'Base', 'data': { 'base1': 'Enum' }, > > + 'if': { 'all': ['IFALL1', 'IFALL2'] } } > > We lack cover for this before your patch. Thanks for fixing it. > > > > > ## > > # @Variant1: > > diff --git a/tests/qapi-schema/doc-good.out > b/tests/qapi-schema/doc-good.out > > index 8f54ceff2e..689d084f3a 100644 > > --- a/tests/qapi-schema/doc-good.out > > +++ b/tests/qapi-schema/doc-good.out > > @@ -12,15 +12,16 @@ enum QType > > module doc-good.json > > enum Enum > > member one > > - if ['defined(IFONE)'] > > + if defined(IFONE) > > member two > > - if ['defined(IFCOND)'] > > + if defined(IFCOND) > > feature enum-feat > > object Base > > member base1: Enum optional=3DFalse > > + if OrderedDict([('all', ['IFALL1', 'IFALL2'])]) > > object Variant1 > > member var1: str optional=3DFalse > > - if ['defined(IFSTR)'] > > + if defined(IFSTR) > > feature member-feat > > feature variant1-feat > > object Variant2 > > @@ -29,7 +30,7 @@ object Object > > tag base1 > > case one: Variant1 > > case two: Variant2 > > - if ['IFTWO'] > > + if IFTWO > > feature union-feat1 > > object q_obj_Variant1-wrapper > > member data: Variant1 optional=3DFalse > > @@ -38,13 +39,13 @@ object q_obj_Variant2-wrapper > > enum SugaredUnionKind > > member one > > member two > > - if ['IFTWO'] > > + if IFTWO > > object SugaredUnion > > member type: SugaredUnionKind optional=3DFalse > > tag type > > case one: q_obj_Variant1-wrapper > > case two: q_obj_Variant2-wrapper > > - if ['IFTWO'] > > + if IFTWO > > feature union-feat2 > > alternate Alternate > > tag type > > diff --git a/tests/qapi-schema/doc-good.txt > b/tests/qapi-schema/doc-good.txt > > index 726727af74..4490108cb7 100644 > > --- a/tests/qapi-schema/doc-good.txt > > +++ b/tests/qapi-schema/doc-good.txt > > @@ -76,6 +76,12 @@ Members > > the first member > > > > > > +If > > +~~ > > + > > +"(IFALL1 and IFALL2)" > > + > > + > > The documentation generated for conditionals is poor before and after > your work. Observation, not demand. > > > "Variant1" (Object) > > ------------------- > > > > [...] > > --0000000000006f91ab05c8b78435 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Tue, Aug 3, 2021 at 5:05 PM Mark= us Armbruster <ar= mbru@redhat.com> wrote:
marcandre.lureau@redhat.com writes:

> From: Marc-Andr=C3=A9 Lureau <marcandre.lureau@redhat.com>
>
> Replace the simple list sugar form with a recursive structure that wil= l
> accept other operators in the following commits (all, any or not).
>
> Signed-off-by: Marc-Andr=C3=A9 Lureau <marcandre.lureau@redhat.com> > ---
>=C2=A0 scripts/qapi/common.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 23 +++++--
>=C2=A0 scripts/qapi/expr.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 52 ++++++++++------
>=C2=A0 scripts/qapi/schema.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 2 +-
>=C2=A0 tests/qapi-schema/bad-if-empty-list.json=C2=A0 =C2=A0 =C2=A0 |= =C2=A0 2 +-
>=C2=A0 tests/qapi-schema/bad-if-list.json=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 |=C2=A0 2 +-
>=C2=A0 tests/qapi-schema/bad-if.err=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 3 +-
>=C2=A0 tests/qapi-schema/doc-good.json=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 3 +-
>=C2=A0 tests/qapi-schema/doc-good.out=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 | 13 ++--
>=C2=A0 tests/qapi-schema/doc-good.txt=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 |=C2=A0 6 ++
>=C2=A0 tests/qapi-schema/enum-if-invalid.err=C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0|=C2=A0 3 +-
>=C2=A0 tests/qapi-schema/features-if-invalid.err=C2=A0 =C2=A0 =C2=A0|= =C2=A0 2 +-
>=C2=A0 tests/qapi-schema/qapi-schema-test.json=C2=A0 =C2=A0 =C2=A0 =C2= =A0| 25 ++++----
>=C2=A0 tests/qapi-schema/qapi-schema-test.out=C2=A0 =C2=A0 =C2=A0 =C2= =A0 | 62 +++++++++----------
>=C2=A0 .../qapi-schema/struct-member-if-invalid.err=C2=A0 |=C2=A0 2 +-<= br> >=C2=A0 .../qapi-schema/union-branch-if-invalid.json=C2=A0 |=C2=A0 2 +-<= br> >=C2=A0 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 @@
>=C2=A0
>=C2=A0 import re
>=C2=A0 from typing import (
> -=C2=A0 =C2=A0 List,
> +=C2=A0 =C2=A0 Any,
> +=C2=A0 =C2=A0 Dict,
>=C2=A0 =C2=A0 =C2=A0 Match,
>=C2=A0 =C2=A0 =C2=A0 Optional,
>=C2=A0 =C2=A0 =C2=A0 Union,
> @@ -199,16 +200,28 @@ def guardend(name: str) -> str:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0na= me=3Dc_fname(name).upper())
>=C2=A0
>=C2=A0
> -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()?=C2=A0 Accident?
<= /blockquote>

Yes, fixed


>=C2=A0 =C2=A0 =C2=A0 if not ifcond:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ''
> -=C2=A0 =C2=A0 return '(' + ') && ('.join(ifco= nd) + ')'
> +=C2=A0 =C2=A0 if isinstance(ifcond, str):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return ifcond
>=C2=A0
> +=C2=A0 =C2=A0 oper, operands =3D next(iter(ifcond.items()))
> +=C2=A0 =C2=A0 oper =3D {'all': ' and '}[oper]
> +=C2=A0 =C2=A0 operands =3D [docgen_ifcond(o) for o in operands]
> +=C2=A0 =C2=A0 return '(' + oper.join(operands) + ')'<= br>
What a nice review speedbump you buried here...

The whole block boils down to the much less exciting

=C2=A0 =C2=A0 =C2=A0 =C2=A0operands =3D [docgen_ifcond(o) for o in ifcond[&= #39;all']]
=C2=A0 =C2=A0 =C2=A0 =C2=A0return '(' + ' and '.join(operan= ds) + ')'

Peeking ahead, I understand that you did it this way here so you can
extend it trivially there.=C2=A0 Matter of taste; what counts is the final<= br> 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.=C2=A0 But please try a bit harder=
to be boring next time ;)

Ah sorry, I d= idn't realize while splitting the patches


>=C2=A0
> -def docgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> +
> +def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
>=C2=A0 =C2=A0 =C2=A0 if not ifcond:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ''
> -=C2=A0 =C2=A0 return ' and '.join(ifcond)
> +=C2=A0 =C2=A0 if isinstance(ifcond, str):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return ifcond

This is what gets rid of the redundant parenthesises in the common case
"single condition string".

> +
> +=C2=A0 =C2=A0 oper, operands =3D next(iter(ifcond.items()))
> +=C2=A0 =C2=A0 oper =3D {'all': '&&'}[oper] > +=C2=A0 =C2=A0 operands =3D [cgen_ifcond(o) for o in operands]
> +=C2=A0 =C2=A0 return '(' + (') ' + oper + ' ('= ;).join(operands) + ')'

This line is hard to read.=C2=A0 Easier, I think:

=C2=A0 =C2=A0 =C2=A0 =C2=A0oper =3D {'all': ' && '}= [oper]
=C2=A0 =C2=A0 =C2=A0 =C2=A0operands =3D ['(' + cgen_ifcond(o) + = 9;)' for o in operands]
=C2=A0 =C2=A0 =C2=A0 =C2=A0return 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

=C2=A0 =C2=A0 #if (defined(TEST_IF_COND_1)) && (defined(TEST_IF_CON= D_2))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 QLIT_QSTR("fea= ture1"),
=C2=A0 =C2=A0 #endif /* (defined(TEST_IF_COND_1)) && (defined(TEST_= IF_COND_2)) */

Mildly annoying.=C2=A0 I'm willing to leave this for later.

I see


Code smell: cgen_ifcond() and docgen_ifcond() are almost identical.=C2=A0 C= an
also be left for later.

ok
<= br>

>=C2=A0
>=C2=A0
>=C2=A0 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: QAPISou= rceInfo) -> None:
>=C2=A0
>=C2=A0 def check_if(expr: _JSONObject, info: QAPISourceInfo, source: st= r) -> None:
>=C2=A0 =C2=A0 =C2=A0 """
> -=C2=A0 =C2=A0 Normalize and validate the ``if`` member of an object.<= br> > +=C2=A0 =C2=A0 Validate the ``if`` member of an object.
>=C2=A0
> -=C2=A0 =C2=A0 The ``if`` member may be either a ``str`` or a ``List[s= tr]``.
> -=C2=A0 =C2=A0 A ``str`` value will be normalized to ``List[str]``. > +=C2=A0 =C2=A0 The ``if`` member may be either a ``str`` or a dict. >=C2=A0
>=C2=A0 =C2=A0 =C2=A0 :forms:
> -=C2=A0 =C2=A0 =C2=A0 :sugared: ``Union[str, List[str]]``
> -=C2=A0 =C2=A0 =C2=A0 :canonical: ``List[str]``
> +=C2=A0 =C2=A0 =C2=A0 :canonical: ``Union[str, dict]``

Does this :forms: thing make sense without any :sugared:?=C2=A0 John, you added (invented?) it in commit a48653638fa, but no explanation made it
into the tree.

>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 :param expr: The expression containing the ``if`` = member to validate.
>=C2=A0 =C2=A0 =C2=A0 :param info: QAPI schema source file information.<= br> > @@ -275,31 +273,45 @@ def check_if(expr: _JSONObject, info: QAPISource= Info, source: str) -> None:
>=C2=A0 =C2=A0 =C2=A0 :raise QAPISemError:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 When the "if" member fails= validation, or when there are no
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 non-empty conditions.
> -=C2=A0 =C2=A0 :return: None, ``expr`` is normalized in-place as neede= d.
> +=C2=A0 =C2=A0 :return: None
>=C2=A0 =C2=A0 =C2=A0 """

Looks like there's a bit more going on than the commit message made me<= br> expect.

>=C2=A0 =C2=A0 =C2=A0 ifcond =3D expr.get('if')
>=C2=A0 =C2=A0 =C2=A0 if ifcond is None:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return
>=C2=A0
> -=C2=A0 =C2=A0 if isinstance(ifcond, list):
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if not ifcond:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QAPISemError(
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 info, "&= #39;if' condition [] of %s is useless" % source)
> -=C2=A0 =C2=A0 else:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 # Normalize to a list
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 ifcond =3D expr['if'] =3D [ifcond= ]
> +=C2=A0 =C2=A0 def _check_if(cond: Union[str, object]) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if isinstance(cond, str):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if not cond.strip():
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QAPISem= Error(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= info,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= "'if' condition '%s' of %s makes no sense"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= % (cond, source))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return
>=C2=A0
> -=C2=A0 =C2=A0 for elt in ifcond:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if not isinstance(elt, str):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if not isinstance(cond, dict):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QAPISemError( >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 info, > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "'if= ' condition of %s must be a string or a list of strings"
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 % source)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if not elt.strip():
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "'if= ' condition of %s must be a string or a dict" % source)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if len(cond) !=3D 1:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QAPISemError( >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 info, > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "'if= ' condition '%s' of %s makes no sense"
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 % (elt, sourc= e))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "'if= ' condition dict of %s must have one key: "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "'al= l'" % source)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 check_keys(cond, info, "'if'= condition", [],
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= ["all"])
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 oper, operands =3D next(iter(cond.items()= ))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if not operands:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QAPISemError(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 info, "&= #39;if' condition [] of %s is useless" % source)
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if oper in ("all") and not isin= stance(operands, list):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QAPISemError(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 info, "&= #39;%s' condition of %s must be a list" % (oper, source))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for operand in operands:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 _check_if(operand)
> +
> +=C2=A0 =C2=A0 _check_if(ifcond)

Putting the function's helper in the middle of the function reminds me<= br> of Mark Twain's "The Awful German Language":

=C2=A0 =C2=A0 "The trunks being now ready, he DE- after kissing his mo= ther and
=C2=A0 =C2=A0 sisters, and once more pressing to his bosom his adored Gretc= hen,
=C2=A0 =C2=A0 who, dressed in simple white muslin, with a single tuberose i= n the
=C2=A0 =C2=A0 ample folds of her rich brown hair, had tottered feebly down = the
=C2=A0 =C2=A0 stairs, still pale from the terror and excitement of the past=
=C2=A0 =C2=A0 evening, but longing to lay her poor aching head yet once aga= in upon
=C2=A0 =C2=A0 the breast of him whom she loved more dearly than life itself= ,
=C2=A0 =C2=A0 PARTED."

I find it hard to read.


Matter of taste, I guess. I'll let= you fix it up to your preference as follow up if you don't mind.
=C2=A0
>=C2=A0
>=C2=A0
>=C2=A0 def normalize_members(members: object) -> None:
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 30d6a01ad1..d2fbdbe583 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -32,7 +32,7 @@
>=C2=A0
>=C2=A0 class QAPISchemaIfCond:
>=C2=A0 =C2=A0 =C2=A0 def __init__(self, ifcond=3DNone):
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.ifcond =3D ifcond or []
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.ifcond =3D ifcond or {}
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 def cgen(self):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return cgen_ifcond(self.ifcond)
> diff --git a/tests/qapi-schema/bad-if-empty-list.json b/tests/qapi-sch= ema/bad-if-empty-list.json
> index 94f2eb8670..b62b5671df 100644
> --- a/tests/qapi-schema/bad-if-empty-list.json
> +++ b/tests/qapi-schema/bad-if-empty-list.json
> @@ -1,3 +1,3 @@
>=C2=A0 # check empty 'if' list
>=C2=A0 { 'struct': 'TestIfStruct', 'data': { &#= 39;foo': 'int' },
> -=C2=A0 'if': [] }
> +=C2=A0 'if': { 'all': [] } }
> diff --git a/tests/qapi-schema/bad-if-list.json b/tests/qapi-schema/ba= d-if-list.json
> index ea3d95bb6b..1fefef16a7 100644
> --- a/tests/qapi-schema/bad-if-list.json
> +++ b/tests/qapi-schema/bad-if-list.json
> @@ -1,3 +1,3 @@
>=C2=A0 # check invalid 'if' content
>=C2=A0 { 'struct': 'TestIfStruct', 'data': { &#= 39;foo': 'int' },
> -=C2=A0 'if': ['foo', ' '] }
> +=C2=A0 'if': { 'all': ['foo', ' '] } = }
> diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.e= rr
> index f83dee65da..8278c49368 100644
> --- a/tests/qapi-schema/bad-if.err
> +++ b/tests/qapi-schema/bad-if.err
> @@ -1,2 +1,3 @@
>=C2=A0 bad-if.json: In struct 'TestIfStruct':
> -bad-if.json:2: 'if' condition of struct must be a string or a= list of strings
> +bad-if.json:2: 'if' condition has unknown key 'value'=
> +Valid keys are 'all'.

"keys are" is awkward when there's just one.=C2=A0 Okay since= there soon will
be more.


right

Test case bad-if.json is meant to cover "value of key 'if' has= an
invalid JSON type".=C2=A0 Before the patch, str and list are valid, an= d the
test uses (invalid) dict.=C2=A0 Afterwards, str and dict are, and the test<= br> still uses (now valid) dict.=C2=A0 In other words, it now tests something else entirely.

I think this test should be updated to something like

=C2=A0 =C2=A0 'if': [ 'defined(TEST_IF_STRUCT)' ]

and a new test added to cover invalid dict key.

ok, and I added some tests to cover new error paths.

> diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-g= ood.json
> index 423ea23e07..25b1053e8a 100644
> --- a/tests/qapi-schema/doc-good.json
> +++ b/tests/qapi-schema/doc-good.json
> @@ -70,7 +70,8 @@
>=C2=A0 # @base1:
>=C2=A0 # the first member
>=C2=A0 ##
> -{ 'struct': 'Base', 'data': { 'base1'= : 'Enum' } }
> +{ 'struct': 'Base', 'data': { 'base1'= : 'Enum' },
> +=C2=A0 'if': { 'all': ['IFALL1', 'IFALL2&= #39;] } }

We lack cover for this before your patch.=C2=A0 Thanks for fixing it.

>=C2=A0
>=C2=A0 ##
>=C2=A0 # @Variant1:
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-go= od.out
> index 8f54ceff2e..689d084f3a 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -12,15 +12,16 @@ enum QType
>=C2=A0 module doc-good.json
>=C2=A0 enum Enum
>=C2=A0 =C2=A0 =C2=A0 member one
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ['defined(IFONE)']
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if defined(IFONE)
>=C2=A0 =C2=A0 =C2=A0 member two
> -=C2=A0 =C2=A0 if ['defined(IFCOND)']
> +=C2=A0 =C2=A0 if defined(IFCOND)
>=C2=A0 =C2=A0 =C2=A0 feature enum-feat
>=C2=A0 object Base
>=C2=A0 =C2=A0 =C2=A0 member base1: Enum optional=3DFalse
> +=C2=A0 =C2=A0 if OrderedDict([('all', ['IFALL1', '= ;IFALL2'])])
>=C2=A0 object Variant1
>=C2=A0 =C2=A0 =C2=A0 member var1: str optional=3DFalse
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ['defined(IFSTR)']
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if defined(IFSTR)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 feature member-feat
>=C2=A0 =C2=A0 =C2=A0 feature variant1-feat
>=C2=A0 object Variant2
> @@ -29,7 +30,7 @@ object Object
>=C2=A0 =C2=A0 =C2=A0 tag base1
>=C2=A0 =C2=A0 =C2=A0 case one: Variant1
>=C2=A0 =C2=A0 =C2=A0 case two: Variant2
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ['IFTWO']
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if IFTWO
>=C2=A0 =C2=A0 =C2=A0 feature union-feat1
>=C2=A0 object q_obj_Variant1-wrapper
>=C2=A0 =C2=A0 =C2=A0 member data: Variant1 optional=3DFalse
> @@ -38,13 +39,13 @@ object q_obj_Variant2-wrapper
>=C2=A0 enum SugaredUnionKind
>=C2=A0 =C2=A0 =C2=A0 member one
>=C2=A0 =C2=A0 =C2=A0 member two
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ['IFTWO']
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if IFTWO
>=C2=A0 object SugaredUnion
>=C2=A0 =C2=A0 =C2=A0 member type: SugaredUnionKind optional=3DFalse
>=C2=A0 =C2=A0 =C2=A0 tag type
>=C2=A0 =C2=A0 =C2=A0 case one: q_obj_Variant1-wrapper
>=C2=A0 =C2=A0 =C2=A0 case two: q_obj_Variant2-wrapper
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ['IFTWO']
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if IFTWO
>=C2=A0 =C2=A0 =C2=A0 feature union-feat2
>=C2=A0 alternate Alternate
>=C2=A0 =C2=A0 =C2=A0 tag type
> diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-go= od.txt
> index 726727af74..4490108cb7 100644
> --- a/tests/qapi-schema/doc-good.txt
> +++ b/tests/qapi-schema/doc-good.txt
> @@ -76,6 +76,12 @@ Members
>=C2=A0 =C2=A0 =C2=A0the first member
>=C2=A0
>=C2=A0
> +If
> +~~
> +
> +"(IFALL1 and IFALL2)"
> +
> +

The documentation generated for conditionals is poor before and after
your work.=C2=A0 Observation, not demand.

>=C2=A0 "Variant1" (Object)
>=C2=A0 -------------------
>=C2=A0

[...]

--0000000000006f91ab05c8b78435--