[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str |
Date: |
Thu, 21 Jan 2021 08:23:05 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
John Snow <jsnow@redhat.com> writes:
> On 1/20/21 11:02 AM, Eric Blake wrote:
>> On 1/20/21 6:07 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Modify visit_module to pass the module itself instead of just its
>>>> name. This allows for future patches to centralize some
>>>> module-interrogation behavior within the QAPISchemaModule class itself,
>>>> cutting down on duplication between gen.py and schema.py.
>>>
>>> We've been tempted to make similar changes before (don't worry, I'm not
>>> building a case for "no" here).
>>>
>>> When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
>>> 2015), I aimed for a loose coupling of backends and the internal
>>> representation. Instead of
>>>
>>> def visit_foo(self, foo):
>>> pass
>>>
>>> where @foo is a QAPISchemaFooBar, I wrote
>>>
>>> def visit_foo_bar(self, name, info, [curated attributes of @foo]):
>>> pass
>>>
>>> In theory, this is nice: the information exposed to the backends is
>>> obvious, and the backends can't accidentally mutate @foo.
>>>
>>> In practice, it kind of failed right then and there:
>>>
>>> def visit_object_type(self, name, info, base, members, variants):
>>> pass
>>>
>>> We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
>>> to pass member information as List[QAPISchemaObjectTypeMember].
>>>
>>> Morever, passing "curated atttibutes" has led to visit_commands() taking
>>> a dozen arguments. Meh.
>>>
>>> This had made Eric and me wonder whether we should write off the
>>> decoupling idea as misguided, and just pass the object instead of
>>> "curated attributes", always. Thoughts?
>> I'm open to the idea of passing just the larger object instead of
>> the
>> curated list of relevant attributes. It's a bit more coupling, but I
>> don't see any of our qapi code being reused outside its current scope
>> where the extra coupling will bite us. But I'm not volunteering for the
>> refactoring work, because I'm not an expert on python typing hints. If
>> consolidating parameters into the larger object makes for fewer
>> parameters and easier typing hints, I'm assuming the work can be done as
>> part of static typing; but if leaving things as they currently are is
>> manageable, that's also fine by me.
>>
>
> Yeah, it can definitely be left as-is for now. I've already gone
> through all the effort of typing out all of the interfaces, so it's
> not really a huge ordeal to just leave it as-is.
>
> Passing the objects might be nicer for the future, though, as routing
> new information or features will involve less churn. (And the
> signatures will be a lot smaller.)
>
> I suppose it does open us up to callers mutating the schema in the
> visitors, but they could already do that for the reasons that Markus
> points out. It's possible that the visitor dispatch could be modified
> to make deep copies of schema objects, but that adds overhead.
>
> I can just revert this change for now and leave the topic for another day.
Works for me. Thanks!
- [PATCH v3 01/17] qapi/commands: assert arg_type is not None, (continued)
[PATCH v3 07/17] qapi/gen: Replace ._begin_system_module(), John Snow, 2021/01/19
[PATCH v3 11/17] qapi: centralize the built-in module name definition, John Snow, 2021/01/19
[PATCH v3 04/17] qapi/gen: inline _wrap_ifcond into end_if(), John Snow, 2021/01/19
[PATCH v3 12/17] qapi/gen: write _genc/_genh access shims, John Snow, 2021/01/19
[PATCH v3 16/17] qapi: type 'info' as Optional[QAPISourceInfo], John Snow, 2021/01/19
[PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module, John Snow, 2021/01/19
[PATCH v3 02/17] qapi/events: fix visit_event typing, John Snow, 2021/01/19