[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree() |
Date: |
Wed, 16 Dec 2020 07:35:02 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
John Snow <jsnow@redhat.com> writes:
> On 11/16/20 4:46 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Returning two different types conditionally can be complicated to
>>> type. Let's always return a tuple for consistency. Prohibit the use of
>>> annotations with dict-values in this circumstance. It can be implemented
>>> later if and when the need for it arises.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/introspect.py | 21 ++++++++++++---------
>>> 1 file changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index 16282f2634b..ef469b6c06e 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -77,14 +77,12 @@
>>>
>>> def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>>> extra: Optional[Annotations] = None
>>> - ) -> TreeValue:
>>> + ) -> Annotated:
>>> if extra is None:
>>> extra = {}
>>> if ifcond:
>>> extra['if'] = ifcond
>>> - if extra:
>>> - return (obj, extra)
>>> - return obj
>>> + return (obj, extra)
>>
>> Less efficient, but that's okay.
>>
>
> I have bad news about Python :)
Well played, sir!
>>>
>>>
>>> def _tree_to_qlit(obj: TreeValue,
>>> @@ -98,12 +96,16 @@ def indent(level: int) -> str:
>>> ifobj, extra = obj
>>> ifcond = cast(Optional[Sequence[str]], extra.get('if'))
>>> comment = extra.get('comment')
>>> +
>>> + msg = "Comments and Conditionals not implemented for dict values"
>>> + assert not (suppress_first_indent and (ifcond or comment)), msg
>>
>> What exactly does this assertion guard?
>>
>
> Something that Eduardo noticed in his review. It's ugly, and I explained
> it poorly.
>
> We don't support annotations on dict *values*, basically. When this
> function is called with suppress_first_indent, we know that we are being
> called to process a dict *value* and not a dict *key*.
>
> What do you do with comments or conditionals on just one half of a key:
> value pair?
>
> Well, break.
Your @msg is a bit misleading then: it suggests comments and
conditionals are not implemented for dict values, but could be. Not
true for conditionals.
Minimally invasive patch correction: drop @msg.
But the actual impossibility to guard against is even simpler, I
believe:
if isinstance(obj, tuple):
assert not suppress_first_indent
But^2, I figure this can be made truly impossible: factor out the part
of _tree_to_qlit() that deals with non-tuple @obj into its own function
(the conditional at its end), then use that for the dict values.
>>> +
>>> ret = ''
>>> if comment:
>>> ret += indent(level) + '/* %s */\n' % comment
>>> if ifcond:
>>> ret += gen_if(ifcond)
>>> - ret += _tree_to_qlit(ifobj, level)
>>> + ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
>>
>> Why do you need to pass on @suppress_first_indent now?
>>
>
> We either never should or we always should have. This is just in the
> case that "suppress first indent" is used on an annotated node. Which,
> err, for the annotations we actually support right now (comment, ifcond)
> -- we will reject in this case.
>
> But it felt precarious...
I suspect the factoring I suggested above will make this less
precarious, too.
>>> if ifcond:
>>> ret += '\n' + gen_endif(ifcond)
>>> return ret
>>> @@ -152,7 +154,7 @@ def __init__(self, prefix: str, unmask: bool):
>>> ' * QAPI/QMP schema introspection', __doc__)
>>> self._unmask = unmask
>>> self._schema: Optional[QAPISchema] = None
>>> - self._trees: List[TreeValue] = []
>>> + self._trees: List[Annotated] = []
>>> self._used_types: List[QAPISchemaType] = []
>>> self._name_map: Dict[str, str] = {}
>>> self._genc.add(mcgen('''
>>> @@ -219,7 +221,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>>>
>>> @classmethod
>>> def _gen_features(cls,
>>> - features: List[QAPISchemaFeature]) ->
>>> List[TreeValue]:
>>> + features: List[QAPISchemaFeature]
>>> + ) -> List[Annotated]:
>>> return [_make_tree(f.name, f.ifcond) for f in features]
>>>
>>> def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>>> @@ -239,7 +242,7 @@ def _gen_tree(self, name: str, mtype: str, obj:
>>> _DObject,
>>> self._trees.append(_make_tree(obj, ifcond, extra))
>>>
>>> def _gen_member(self,
>>> - member: QAPISchemaObjectTypeMember) -> TreeValue:
>>> + member: QAPISchemaObjectTypeMember) -> Annotated:
>>> obj: _DObject = {
>>> 'name': member.name,
>>> 'type': self._use_type(member.type)
>>> @@ -255,7 +258,7 @@ def _gen_variants(self, tag_name: str,
>>> return {'tag': tag_name,
>>> 'variants': [self._gen_variant(v) for v in variants]}
>>>
>>> - def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
>>> + def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
>>> obj: _DObject = {
>>> 'case': variant.name,
>>> 'type': self._use_type(variant.type)