Re: [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 1/20/21 9:20 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
[...]
>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>>> index 55acd7e080d..b5505685e6e 100644
>>> --- a/scripts/qapi/gen.py
>>> +++ b/scripts/qapi/gen.py
[...]
>>> @@ -313,7 +306,8 @@ def visit_module(self, module: QAPISchemaModule) -> 
>>> None:
>>>                   self._genc = None
>>>                   self._genh = None
>>>           else:
>>> -            self._add_user_module(module.name, self._user_blurb)
>>> +            assert module.user_module, "Unexpected system module"
>> The string provides no value.
>> 
>
> That's just, like, your opinion, man. It has value to me.
>
>
> Compare:
>
> ```
> #!/usr/bin/env python3
>
> def main():
>     assert False
>
> if __name__ == '__main__':
>     main()
> ```
>
> ```
> # ./foo.py
>
> Traceback (most recent call last):
>   File "./foo.py", line 7, in <module>
>     main()
>   File "./foo.py", line 4, in main
>     assert False
> AssertionError
> ```
>
> With:
>
>
> ```
> #!/usr/bin/env python3
>
> def main():
>     assert False, "Unexpected system module"
>
> if __name__ == '__main__':
>     main()
> ```
>
> ```
> # ./foo.py
>
> Traceback (most recent call last):
>   File "./foo.py", line 7, in <module>
>     main()
>   File "./foo.py", line 4, in main
>     assert False, "Unexpected system module"
> AssertionError: Unexpected system module
> ```
>
> I like the extra string for giving some semantic context as to
> specifically what broke (We don't expect to see system modules here) 
> instead of just a stack trace.

Your test uses assert with an argument that tells us nothing.  But the
assert we're arguing about has a nice, expressive argument.

The string improves the report from

      File "/work/armbru/qemu/scripts/qapi/gen.py", line 325, in visit_module
        assert module.user_module
    AssertionError

to

      File "/work/armbru/qemu/scripts/qapi/gen.py", line 325, in visit_module
        assert module.user_module, "Unexpected system module"
    AssertionError: Unexpected system module

Even if you value the difference, I very much doubt it justifies the
clutter.  Also, slippery slope towards pigs wearing way too much
lipstick.

Tested with

    diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
    index f3f4d2b011..bbfb30dc5a 100644
    --- a/scripts/qapi/gen.py
    +++ b/scripts/qapi/gen.py
    @@ -321,6 +321,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor):
                     # be generated.
                     self._current_module = None
             else:
    +            module.name = "./HACK"
                 assert module.user_module, "Unexpected system module"
                 self._add_module(module.name, self._user_blurb)
                 self._begin_user_module(module.name)


>
>>> +            self._add_module(module.name, self._user_blurb)
>>>               self._begin_user_module(module.name)
>>>         def visit_include(self, name: str, info: QAPISourceInfo) ->
>>> None: