Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/16/20 3:26 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> We already assert this in end_if, but that's opaque to mypy. Do it in
>>> _wrap_ifcond instead. Same effect at runtime, but mypy can now infer
>>> the type in _wrap_ifcond's body.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/gen.py | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>>> index b40f18eee3cd..a6dc991b1d03 100644
>>> --- a/scripts/qapi/gen.py
>>> +++ b/scripts/qapi/gen.py
>>> @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
>>>           self._start_if = (ifcond, self._body, self._preamble)
>>>   
>>>       def end_if(self) -> None:
>>> -        assert self._start_if
>>>           self._wrap_ifcond()
>>>           self._start_if = None
>>>   
>>>       def _wrap_ifcond(self) -> None:
>>> +        assert self._start_if
>>>           self._body = _wrap_ifcond(self._start_if[0],
>>>                                     self._start_if[1], self._body)
>>>           self._preamble = _wrap_ifcond(self._start_if[0],
>> 
>> Drawback: the public method's precondition is now more opaque.  Do we
>> care?
>> 
>
> Ish. If you call end_if before start_if, what did you want to have happen?

Point.

> Or more to the point: do you want the assertion in both places?

What about inlining QAPIGenCCode._wrap_ifcond() into .end_if()?