Re: [Qemu-devel] [PATCH v3 28/50] qapi: add 'if' to alternate variant

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  scripts/qapi.py                         | 9 ++++++++-
>  tests/qapi-schema/qapi-schema-test.json | 6 +++++-
>  tests/qapi-schema/qapi-schema-test.out  | 8 +++++++-
>  3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 2f14edfa1f..19e48bd4d2 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -849,6 +849,9 @@ def check_alternate(expr, info):
       for (key, value) in members.items():
           check_name(info, "Member of alternate '%s'" % name, key)

           # Ensure alternates have no type conflicts.
>          check_type(info, "Member '%s' of alternate '%s'" % (key, name),
>                     value,
>                     allow_metas=['built-in', 'union', 'struct', 'enum'])
> +        if isinstance(value, dict):
> +            check_unknown_keys(info, value, {'type', 'if'})
> +            value = value['type']
>          qtype = find_alternate_member_qtype(value)
>          if not qtype:
>              raise QAPISemError(info, "Alternate '%s' member '%s' cannot use "

I stared at this some because I couldn't see the check_if().  Looks like
it's done in check_type().  I guess I'm hitting the limits of what I can
do in *patch* review, and to do better, I'd have to go through the
resulting code with a fine comb.

You generalize the right hand side from "TYPE" to {"type": "TYPE", "if:
"IFCOND"}, where "if" is optional.  The old "TYPE" becomes syntactic
sugar for {"type": "TYPE"}.  Two remarks:

* Since the right hand side is more than just a type, the name
  check_type() is now inappropriate.  Since PATCH 23.

* The sane way to do syntactic sugar is to desugar at the first
  opportunity.  You do the opposite: you handle the new aspect of the
  general form first, and then rewrite it to the sugared form for
  further processing, most probably to reduce churn.  Reducing churn is
  good, but I find the resulting code hard to follow, because @value
  changes meaning halfway through.  May well apply to the other similar
  patches, too.

> @@ -1671,7 +1674,11 @@ class QAPISchema(object):
>                                                None))
>  
>      def _make_variant(self, case, typ):
> -        return QAPISchemaObjectTypeVariant(case, typ)
> +        ifcond = None
> +        if isinstance(typ, dict):
> +            ifcond = typ.get('if')
> +            typ = typ['type']
> +        return QAPISchemaObjectTypeVariant(case, typ, ifcond)
>  
>      def _make_simple_variant(self, case, typ, info):
>          ifcond = None
> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index 895e80a978..f7a62b24d1 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -208,9 +208,13 @@
>  { 'command': 'TestIfUnionCmd', 'data': { 'union_cmd_arg': 'TestIfUnion' },
>    'if': 'defined(TEST_IF_UNION)' }
>  
> -{ 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar': 
> 'TestStruct' },
> +{ 'alternate': 'TestIfAlternate', 'data':
> +  { 'foo': 'int', 'alt_bar': { 'type': 'TestStruct', 'if': 
> 'defined(TEST_IF_ALT_BAR)'} },
>    'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
>  
> +{ 'command': 'TestIfAlternateCmd', 'data': { 'alt_cmd_arg': 
> 'TestIfAlternate' },
> +  'if': 'defined(TEST_IF_ALT)' }
> +
>  { 'command': 'TestIfCmd', 'data':
>    { 'foo': 'TestIfStruct',
>      'bar': { 'type': 'TestIfEnum', 'if': 'defined(TEST_IF_CMD_BAR)' } },
> diff --git a/tests/qapi-schema/qapi-schema-test.out 
> b/tests/qapi-schema/qapi-schema-test.out
> index ee009c5626..6887e49c9b 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -67,8 +67,11 @@ enum QType
>  alternate TestIfAlternate
>      tag type
>      case foo: int
> -    case bar: TestStruct
> +    case alt_bar: TestStruct if=defined(TEST_IF_ALT_BAR)
>      if defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)
> +command TestIfAlternateCmd q_obj_TestIfAlternateCmd-arg -> None
> +   gen=True success_response=True boxed=False
> +    if defined(TEST_IF_ALT)
>  command TestIfCmd q_obj_TestIfCmd-arg -> None
>     gen=True success_response=True boxed=False
>      if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)
> @@ -232,6 +235,9 @@ object q_obj_EVENT_D-arg
>      member b: str optional=False
>      member c: str optional=True
>      member enum3: EnumOne optional=True
> +object q_obj_TestIfAlternateCmd-arg
> +    member alt_cmd_arg: TestIfAlternate optional=False
> +    if defined(TEST_IF_ALT)
>  object q_obj_TestIfCmd-arg
>      member foo: TestIfStruct optional=False
>      member bar: TestIfEnum optional=False if=defined(TEST_IF_CMD_BAR)