| Message ID | 20210618102507.3761128-8-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: untie 'if' conditions from C preprocessor | expand |
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. > > :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 = 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) 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. > > > 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=None): > - self.ifcond = ifcond or [] > + self.ifcond = 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.err > 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. 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. > 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=False > + if OrderedDict([('all', ['IFALL1', 'IFALL2'])]) > object Variant1 > member var1: str optional=False > - 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=False > @@ -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=False > 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) > ------------------- > [...]
One more thing... 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> > --- [...] > 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]`` > > :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) Not covered by the negative tests in tests/qapi-schema/. Please double-check that all new errors are covered. > + 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) > > [...]
Hi On Tue, Aug 3, 2021 at 5:05 PM 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? > Yes, fixed > > 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 ;) > 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 = 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. > 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 = 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) > > 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=None): > > - self.ifcond = ifcond or [] > > + self.ifcond = 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.err > > 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=False > > + if OrderedDict([('all', ['IFALL1', 'IFALL2'])]) > > object Variant1 > > member var1: str optional=False > > - 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=False > > @@ -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=False > > 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) > > ------------------- > > > > [...] > >
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
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: 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) + ')' 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]`` :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) 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=None): - self.ifcond = ifcond or [] + self.ifcond = 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.err 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'. 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'] } } ## # @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=False + if OrderedDict([('all', ['IFALL1', 'IFALL2'])]) object Variant1 member var1: str optional=False - 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=False @@ -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=False 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)" + + "Variant1" (Object) ------------------- diff --git a/tests/qapi-schema/enum-if-invalid.err b/tests/qapi-schema/enum-if-invalid.err index 0556dc967b..df305cd79f 100644 --- a/tests/qapi-schema/enum-if-invalid.err +++ b/tests/qapi-schema/enum-if-invalid.err @@ -1,2 +1,3 @@ enum-if-invalid.json: In enum 'TestIfEnum': -enum-if-invalid.json:2: 'if' condition of 'data' member 'bar' must be a string or a list of strings +enum-if-invalid.json:2: 'if' condition has unknown key 'val' +Valid keys are 'all'. diff --git a/tests/qapi-schema/features-if-invalid.err b/tests/qapi-schema/features-if-invalid.err index f63b89535e..8b64318df6 100644 --- a/tests/qapi-schema/features-if-invalid.err +++ b/tests/qapi-schema/features-if-invalid.err @@ -1,2 +1,2 @@ features-if-invalid.json: In struct 'Stru': -features-if-invalid.json:2: 'if' condition of 'features' member 'f' must be a string or a list of strings +features-if-invalid.json:2: 'if' condition of 'features' member 'f' must be a string or a dict diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 84b9d41f15..f2e0fff51f 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -231,8 +231,8 @@ { 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct', - 'bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} }, - 'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' } + 'union-bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} }, + 'if': { 'all': ['defined(TEST_IF_UNION)', 'defined(TEST_IF_STRUCT)'] } } { 'command': 'test-if-union-cmd', 'data': { 'union-cmd-arg': 'TestIfUnion' }, @@ -241,25 +241,24 @@ { 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar': { 'type': 'TestStruct', 'if': 'defined(TEST_IF_ALT_BAR)'} }, - 'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' } + 'if': { 'all': ['defined(TEST_IF_ALT)', 'defined(TEST_IF_STRUCT)'] } } -{ 'command': 'test-if-alternate-cmd', - 'data': { 'alt-cmd-arg': 'TestIfAlternate' }, - 'if': 'defined(TEST_IF_ALT)' } +{ 'command': 'test-if-alternate-cmd', 'data': { 'alt-cmd-arg': 'TestIfAlternate' }, + 'if': { 'all': ['defined(TEST_IF_ALT)'] } } { 'command': 'test-if-cmd', 'data': { 'foo': 'TestIfStruct', 'bar': { 'type': 'TestIfEnum', 'if': 'defined(TEST_IF_CMD_BAR)' } }, 'returns': 'UserDefThree', - 'if': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] } + 'if': { 'all': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] } } { 'command': 'test-cmd-return-def-three', 'returns': 'UserDefThree' } { 'event': 'TEST_IF_EVENT', 'data': { 'foo': 'TestIfStruct', 'bar': { 'type': ['TestIfEnum'], 'if': 'defined(TEST_IF_EVT_BAR)' } }, - 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' } + 'if': { 'all': ['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'] } } # test 'features' @@ -288,8 +287,9 @@ { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] } { 'struct': 'CondFeatureStruct3', 'data': { 'foo': 'int' }, - 'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)', - 'defined(TEST_IF_COND_2)'] } ] } + 'features': [ { 'name': 'feature1', + 'if': { 'all': [ 'defined(TEST_IF_COND_1)', + 'defined(TEST_IF_COND_2)'] } } ] } { 'enum': 'FeatureEnum1', 'data': [ 'eins', 'zwei', 'drei' ], @@ -328,8 +328,9 @@ 'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'}, { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] } { 'command': 'test-command-cond-features3', - 'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)', - 'defined(TEST_IF_COND_2)'] } ] } + 'features': [ { 'name': 'feature1', + 'if': { 'all': [ 'defined(TEST_IF_COND_1)', + 'defined(TEST_IF_COND_2)'] } } ] } { 'event': 'TEST_EVENT_FEATURES0', 'data': 'FeatureStruct1' } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index e0b8a5f0b6..6a1b3aa341 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -298,65 +298,65 @@ command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Unio object TestIfStruct member foo: int optional=False member bar: int optional=False - if ['defined(TEST_IF_STRUCT_BAR)'] - if ['defined(TEST_IF_STRUCT)'] + if defined(TEST_IF_STRUCT_BAR) + if defined(TEST_IF_STRUCT) enum TestIfEnum member foo member bar - if ['defined(TEST_IF_ENUM_BAR)'] - if ['defined(TEST_IF_ENUM)'] + if defined(TEST_IF_ENUM_BAR) + if defined(TEST_IF_ENUM) object q_obj_TestStruct-wrapper member data: TestStruct optional=False enum TestIfUnionKind member foo - member bar - if ['defined(TEST_IF_UNION_BAR)'] - if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)'] + member union-bar + if defined(TEST_IF_UNION_BAR) + if OrderedDict([('all', ['defined(TEST_IF_UNION)', 'defined(TEST_IF_STRUCT)'])]) object TestIfUnion member type: TestIfUnionKind optional=False tag type case foo: q_obj_TestStruct-wrapper - case bar: q_obj_str-wrapper - if ['defined(TEST_IF_UNION_BAR)'] - if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)'] + case union-bar: q_obj_str-wrapper + if defined(TEST_IF_UNION_BAR) + if OrderedDict([('all', ['defined(TEST_IF_UNION)', 'defined(TEST_IF_STRUCT)'])]) object q_obj_test-if-union-cmd-arg member union-cmd-arg: TestIfUnion optional=False - if ['defined(TEST_IF_UNION)'] + if defined(TEST_IF_UNION) command test-if-union-cmd q_obj_test-if-union-cmd-arg -> None gen=True success_response=True boxed=False oob=False preconfig=False - if ['defined(TEST_IF_UNION)'] + if defined(TEST_IF_UNION) alternate TestIfAlternate tag type case foo: int case bar: TestStruct - if ['defined(TEST_IF_ALT_BAR)'] - if ['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)'] + if defined(TEST_IF_ALT_BAR) + if OrderedDict([('all', ['defined(TEST_IF_ALT)', 'defined(TEST_IF_STRUCT)'])]) object q_obj_test-if-alternate-cmd-arg member alt-cmd-arg: TestIfAlternate optional=False - if ['defined(TEST_IF_ALT)'] + if OrderedDict([('all', ['defined(TEST_IF_ALT)'])]) command test-if-alternate-cmd q_obj_test-if-alternate-cmd-arg -> None gen=True success_response=True boxed=False oob=False preconfig=False - if ['defined(TEST_IF_ALT)'] + if OrderedDict([('all', ['defined(TEST_IF_ALT)'])]) object q_obj_test-if-cmd-arg member foo: TestIfStruct optional=False member bar: TestIfEnum optional=False - if ['defined(TEST_IF_CMD_BAR)'] - if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] + if defined(TEST_IF_CMD_BAR) + if OrderedDict([('all', ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'])]) command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree gen=True success_response=True boxed=False oob=False preconfig=False - if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] + if OrderedDict([('all', ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'])]) command test-cmd-return-def-three None -> UserDefThree gen=True success_response=True boxed=False oob=False preconfig=False array TestIfEnumList TestIfEnum - if ['defined(TEST_IF_ENUM)'] + if defined(TEST_IF_ENUM) object q_obj_TEST_IF_EVENT-arg member foo: TestIfStruct optional=False member bar: TestIfEnumList optional=False - if ['defined(TEST_IF_EVT_BAR)'] - if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)'] + if defined(TEST_IF_EVT_BAR) + if OrderedDict([('all', ['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'])]) event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg boxed=False - if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)'] + if OrderedDict([('all', ['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'])]) object FeatureStruct0 member foo: int optional=False object FeatureStruct1 @@ -379,17 +379,17 @@ object FeatureStruct4 object CondFeatureStruct1 member foo: int optional=False feature feature1 - if ['defined(TEST_IF_FEATURE_1)'] + if defined(TEST_IF_FEATURE_1) object CondFeatureStruct2 member foo: int optional=False feature feature1 - if ['defined(TEST_IF_FEATURE_1)'] + if defined(TEST_IF_FEATURE_1) feature feature2 - if ['defined(TEST_IF_FEATURE_2)'] + if defined(TEST_IF_FEATURE_2) object CondFeatureStruct3 member foo: int optional=False feature feature1 - if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'] + if OrderedDict([('all', ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'])]) enum FeatureEnum1 member eins member zwei @@ -429,17 +429,17 @@ command test-command-features3 None -> None command test-command-cond-features1 None -> None gen=True success_response=True boxed=False oob=False preconfig=False feature feature1 - if ['defined(TEST_IF_FEATURE_1)'] + if defined(TEST_IF_FEATURE_1) command test-command-cond-features2 None -> None gen=True success_response=True boxed=False oob=False preconfig=False feature feature1 - if ['defined(TEST_IF_FEATURE_1)'] + if defined(TEST_IF_FEATURE_1) feature feature2 - if ['defined(TEST_IF_FEATURE_2)'] + if defined(TEST_IF_FEATURE_2) command test-command-cond-features3 None -> None gen=True success_response=True boxed=False oob=False preconfig=False feature feature1 - if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'] + if OrderedDict([('all', ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'])]) event TEST_EVENT_FEATURES0 FeatureStruct1 boxed=False event TEST_EVENT_FEATURES1 None diff --git a/tests/qapi-schema/struct-member-if-invalid.err b/tests/qapi-schema/struct-member-if-invalid.err index 42e7fdae3c..eea4c62aaf 100644 --- a/tests/qapi-schema/struct-member-if-invalid.err +++ b/tests/qapi-schema/struct-member-if-invalid.err @@ -1,2 +1,2 @@ struct-member-if-invalid.json: In struct 'Stru': -struct-member-if-invalid.json:2: 'if' condition of 'data' member 'member' must be a string or a list of strings +struct-member-if-invalid.json:2: 'if' condition of 'data' member 'member' must be a string or a dict diff --git a/tests/qapi-schema/union-branch-if-invalid.json b/tests/qapi-schema/union-branch-if-invalid.json index 46d4239af6..c41633856f 100644 --- a/tests/qapi-schema/union-branch-if-invalid.json +++ b/tests/qapi-schema/union-branch-if-invalid.json @@ -3,4 +3,4 @@ { 'struct': 'Stru', 'data': { 'member': 'str' } } { 'union': 'Uni', 'base': { 'tag': 'Branches' }, 'discriminator': 'tag', - 'data': { 'branch1': { 'type': 'Stru', 'if': [''] } } } + 'data': { 'branch1': { 'type': 'Stru', 'if': { 'all': [''] } } } }