diff mbox series

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

Message ID 20200922210101.4081073-32-jsnow@redhat.com
State Superseded
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Sept. 22, 2020, 9 p.m. UTC
_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(-)

Comments

Eduardo Habkost Sept. 23, 2020, 4:35 p.m. UTC | #1
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
John Snow Sept. 23, 2020, 9:43 p.m. UTC | #2
On 9/23/20 12:35 PM, Eduardo Habkost wrote:
> I believe these two lines above should be removed, as suggested

> in patch 30, but let's ignore that for now.

> 


Yup, headed there.

>> -    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.


The basic premise is:

Why pass information you want to add to obj['features'] to a function to 
make that assignment, when you could just perform that assignment yourself?

Otherwise, _make_tree, which accepts any arbitrary object (not just 
dicts!) has to interrogate its arguments to make sure you gave it a dict 
when you give it a features argument.

Type-wise, it's cleaner to perform this transformation when we KNOW we 
have an object than it is to defer to a more abstracted function and 
assert/downcast back to more specific types.
Eduardo Habkost Sept. 23, 2020, 9:54 p.m. UTC | #3
On Wed, Sep 23, 2020 at 05:43:45PM -0400, John Snow wrote:
> On 9/23/20 12:35 PM, Eduardo Habkost wrote:
> > I believe these two lines above should be removed, as suggested
> > in patch 30, but let's ignore that for now.
> > 
> 
> Yup, headed there.
> 
> > > -    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.
> 
> The basic premise is:
> 
> Why pass information you want to add to obj['features'] to a function to
> make that assignment, when you could just perform that assignment yourself?
> 
> Otherwise, _make_tree, which accepts any arbitrary object (not just dicts!)
> has to interrogate its arguments to make sure you gave it a dict when you
> give it a features argument.

Oh, I was not aware that obj could be not a dictionary.  In this
case, it makes lots of sense to move the magic outside the
function.

> 
> Type-wise, it's cleaner to perform this transformation when we KNOW we have
> an object than it is to defer to a more abstracted function and
> assert/downcast back to more specific types.
diff mbox series

Patch

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)
-    if features:
-        obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in features]
     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)