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

[John Snow]

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.