[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] qapi: introduce forwarding visitor

From: Paolo Bonzini
Subject: Re: [PATCH 1/2] qapi: introduce forwarding visitor
Date: Fri, 23 Jul 2021 11:49:04 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

On 22/07/21 17:34, Markus Armbruster wrote:
This is not a fifth type of visitor, it's a wrapper for the existing
types (two of them, input and output; the other two don't break
horribly but make no sense either).

Unlike the other visitors, this one isn't of a fixed type.  I think
mentioning this would be nice.  Perhaps add to the paragraph

Ah okay, I didn't understand that paragraph referred to the actual visitors and not just the kinds in the enum.

Can you explain why you treat names in sub-structs differently than
names other than the alias name in the root struct?

Taking the example of QOM alias properties, if the QOM property you're
aliasing is a struct, its field names are irrelevant.  The caller may
not even know what they are, as they are not part of the namespace (e.g.
the toplevel QDict returned by keyval_parse) that is being modified.

There are no aliased compound QOM properties that I can make a proper
example with, unfortunately.

Since the intent is to forward *only* the alias, I wonder why we forward
*everything* when v->depth > 0.

Oh.  Is it because to get to v->depth > 0, we must have entered the
alias, so whatever we forward there must be members of the alias?

Yes, exactly. v->depth is only nonzero after the name translation has succeeded (and until end_struct/end_list).

+Visitor *visitor_forward_field(Visitor *target, const char *from, const char 
+    ForwardFieldVisitor *v = g_new0(ForwardFieldVisitor, 1);
+    v->visitor.type = target->type;

Do arbitrary types work?  Or is this limited to input and output

They don't crash, but they don't make sense because 1) they should not
live outside qapi_clone and visit_free_* 2) they use NULL for the
outermost name.

I'd prefer to restrict the forwarding visitor to the cases that make
sense and have test coverage.

Yup, I had added an assertion in the incremental diff already.

Not forwarded: method .type_size().  Impact: visit_type_size() will call
the wrapped visitor's .type_uint64() instead of its .type_size().  The
two differ for the opts visitor, the keyval input visitor, the string
input visitor, and the string output visitor.

Fixed, of course.  Incremental diff after my sig.

Looks good to me apart from rather long lines in block comments.
Best to wrap these around column 70, unless the wrapping obviously
reduces legibility.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]