Re: [PATCH v2 31/38] qapi/introspect.py: add _gen_features helper

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:00:54PM -0400, John Snow wrote:
> _make_tree doesn't know if it is receiving an object or some other type;
> adding features information should arguably be performed by the caller.
> 
> This will help us refactor _make_tree more gracefully in the next patch.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/introspect.py | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 41ca8afc67..e1edd0b179 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -10,7 +10,7 @@
>  See the COPYING file in the top-level directory.
>  """
>  
> -from typing import (NamedTuple, Optional, Sequence)
> +from typing import (NamedTuple, List, Optional, Sequence)
>  
>  from .common import (
>      c_name,
> @@ -20,7 +20,7 @@
>  )
>  from .gen import QAPISchemaMonolithicCVisitor
>  from .schema import (QAPISchemaArrayType, QAPISchemaBuiltinType,
> -                     QAPISchemaType)
> +                     QAPISchemaFeature, QAPISchemaType)
>  
>  
>  class Extra(NamedTuple):
> @@ -31,12 +31,10 @@ class Extra(NamedTuple):
>      ifcond: Sequence[str] = tuple()
>  
>  
> -def _make_tree(obj, ifcond, features,
> +def _make_tree(obj, ifcond,
>                 extra: Optional[Extra] = None):
>      comment = extra.comment if extra else None
>      extra = Extra(comment, ifcond)

I believe these two lines above should be removed, as suggested
in patch 30, but let's ignore that for now.

> -    if features:
> -        obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in features]

I can't say I understand completely why moving these two lines
outside _make_tree() is useful, but if it makes the cleanup work
you did easier, I trust this is the right thing to do.  The
changes look correct.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


>      return (obj, extra)
>  
>  
> @@ -169,6 +167,10 @@ def _use_type(self, typ):
>              return '[' + self._use_type(typ.element_type) + ']'
>          return self._name(typ.name)
>  
> +    @classmethod
> +    def _gen_features(cls, features: List[QAPISchemaFeature]):
> +        return [_make_tree(f.name, f.ifcond) for f in features]
> +
>      def _gen_tree(self, name, mtype, obj, ifcond, features):
>          extra = None
>          if mtype not in ('command', 'event', 'builtin', 'array'):
> @@ -179,13 +181,17 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>              name = self._name(name)
>          obj['name'] = name
>          obj['meta-type'] = mtype
> -        self._trees.append(_make_tree(obj, ifcond, features, extra))
> +        if features:
> +            obj['features'] = self._gen_features(features)
> +        self._trees.append(_make_tree(obj, ifcond, extra))
>  
>      def _gen_member(self, member):
>          obj = {'name': member.name, 'type': self._use_type(member.type)}
>          if member.optional:
>              obj['default'] = None
> -        return _make_tree(obj, member.ifcond, member.features)
> +        if member.features:
> +            obj['features'] = self._gen_features(member.features)
> +        return _make_tree(obj, member.ifcond)
>  
>      def _gen_variants(self, tag_name, variants):
>          return {'tag': tag_name,
> @@ -193,7 +199,7 @@ def _gen_variants(self, tag_name, variants):
>  
>      def _gen_variant(self, variant):
>          obj = {'case': variant.name, 'type': self._use_type(variant.type)}
> -        return _make_tree(obj, variant.ifcond, None)
> +        return _make_tree(obj, variant.ifcond)
>  
>      def visit_builtin_type(self, name, info, json_type):
>          self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
> -- 
> 2.26.2
> 

-- 
Eduardo