Re: [PATCH v4 39/46] qapi/introspect.py: Unify return type of _make_tree()

[John Snow <jsnow@redhat.com>]

On 9/30/20 2:24 PM, Eduardo Habkost wrote:
> On Wed, Sep 30, 2020 at 12:31:43AM -0400, John Snow wrote:
>> Returning a *something* or a Tuple of *something* is hard to accurately
>> type. Let's just always return a tuple for structural consistency.
>>
>> Instances of the 'TreeNode' type can be replaced with the slightly more
>> specific 'AnnotatedNode' type.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> So, the only place where this seems to make a difference is
> _tree_to_qlit().
> 
> We just need to prove that
>    _tree_to_qlit(o, ...)
> will have exactly the same result as
>    _tree_to_qlit((o, None), ...).
> 
> For reference, this is the beginning of _tree_to_qlit():
> 
> | def _tree_to_qlit(obj: TreeNode,
> |                   level: int = 0,
> |                   suppress_first_indent: bool = False) -> str:
> |
> |     def indent(level: int) -> str:
> |         return level * 4 * ' '
> |
> |     if isinstance(obj, tuple):
> |         ifobj, extra = obj
> 
> `obj` is the return value of _make_tree()
> 
> `ifobj` is the original `obj` argument to _make_tree().
> 
> |         ifcond = extra.get('if')
> 
> ifcond will be None.
> 
> |         comment = extra.get('comment')
> 
> comment will be None
> 
> |         ret = ''
> |         if comment:
> |             ret += indent(level) + '/* %s */\n' % comment
> 
> nop
> 
> |         if ifcond:
> |             ret += gen_if(ifcond)
> 
> nop
> 
> |         ret += _tree_to_qlit(ifobj, level)
> 
> ret will be '', so this is equivalent to:
> 
>    ret = _tree_to_qlit(ifobj, level)
> 
> which is almost good.
> 
> The only difference seems to that suppress_first_indent=True will
> be ignored.  We should pass suppress_first_indent as argument in
> the recursive call above, just in case.
> 

This is a really good spot, and I indeed hadn't considered it at all 
when I did this.

(I simply made the change and observed it worked just fine!)

> The existing code will behave weirdly if there are comments or
> conditions and suppress_first_indent=True, but I suggest we try
> to address this issue later.
> 
> |         if ifcond:
> |             ret += '\n' + gen_endif(ifcond)
> 
> nop
> 
> |         return ret
> 

Hm, yes, it's a hypothetical case, but perhaps we can use an assertion 
to help guard against it if development creates that case later by accident.

That ought to be good enough for now to not waste time accommodating a 
(presently) fictional circumstance?

Thanks for the good sleuthing here.

--js