Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str

From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>,
	qemu-devel@nongnu.org, "Eduardo Habkost" <ehabkost@redhat.com>,
	"Cleber Rosa" <crosa@redhat.com>
Subject: Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str
Date: Thu, 21 Jan 2021 08:23:05 +0100	[thread overview]
Message-ID: <87ft2ulpnq.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <000bd3b7-1342-d26a-5cc9-3191efe0b0ec@redhat.com> (John Snow's message of "Wed, 20 Jan 2021 11:16:19 -0500")

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!