Re: [PATCH v2 08/11] qapi/introspect.py: replace 'extra' dict with 'comment' argument

[John Snow]

On 11/16/20 4:55 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > This is only used to pass in a dictionary with a comment already set, so
> > skip the runaround and just accept the comment.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/introspect.py | 17 ++++++++---------
> >   1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index ef469b6c06e..a0978cb3adb 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -76,12 +76,11 @@
> >   
> >   
> >   def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
> > -               extra: Optional[Annotations] = None
> > -               ) -> Annotated:
> > -    if extra is None:
> > -        extra = {}
> > -    if ifcond:
> > -        extra['if'] = ifcond
> > +               comment: Optional[str] = None) -> Annotated:
> > +    extra: Annotations = {
> > +        'if': ifcond,
> > +        'comment': comment,
> > +    }
>
> Works because _tree_to_qlit() treats 'if': None, 'comment': None exactly
> like absent 'if', 'comment'.  Mentioning this in the commit message
> wouldn't hurt.

OK.

> We create even more dicts now.  Okay.

It's just shuffling around where this dict is made, I don't think we 
create more in total.

> >       return (obj, extra)
> >   
> >   
> > @@ -228,18 +227,18 @@ def _gen_features(cls,
> >       def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> >                     ifcond: List[str],
> >                     features: Optional[List[QAPISchemaFeature]]) -> None:
> > -        extra: Optional[Annotations] = None
> > +        comment: Optional[str] = None
> >           if mtype not in ('command', 'event', 'builtin', 'array'):
> >               if not self._unmask:
> >                   # Output a comment to make it easy to map masked names
> >                   # back to the source when reading the generated output.
> > -                extra = {'comment': '"%s" = %s' % (self._name(name), name)}
> > +                comment = f'"{self._name(name)}" = {name}'
>
> Drive-by modernization, fine with me.  Aside: many more opportunities; a
> systematic hunt is called for.  Not now.

It happened because of line-length limits, admittedly.

> >               name = self._name(name)
> >           obj['name'] = name
> >           obj['meta-type'] = mtype
> >           if features:
> >               obj['features'] = self._gen_features(features)
> > -        self._trees.append(_make_tree(obj, ifcond, extra))
> > +        self._trees.append(_make_tree(obj, ifcond, comment))
> >   
> >       def _gen_member(self,
> >                       member: QAPISchemaObjectTypeMember) -> Annotated: