On 05/06/2016 06:31 AM, Markus Armbruster wrote: >> So all that's left are the two output functions. Can we get rid >> of those, and make Visitor* the only public interface, rather than >> making every caller have to do upcasts? > > The two output functions are > > QObject *qmp_output_get_qobject(QmpOutputVisitor *v); > char *string_output_get_string(StringOutputVisitor *v); > >> It looks like outside of the testsuite, all uses of these visitors are >> local to a single function; and improving the testsuite is not the end >> of the world. Particularly if only the testsuite is using reset, it may >> be easier to just patch the testsuite to use a new visitor in the places >> where it currently does a reset. > > I'm okay with replacing reset by destroy + new in the test suite.
That part's (relatively) easy, so it will be in the next spin.
>> QObject *object_property_get_qobject(Object *obj, const char *name,
>> Error **errp)
>> {
>> QObject *ret = NULL;
>> Error *local_err = NULL;
>> Visitor *v;
>>
>> v = qmp_output_visitor_new(&ret);
>> object_property_get(obj, v, name, &local_err);
>> if (!local_err) {
>> visit_complete(v); /* populates ret */
>> }
>> error_propagate(errp, local_err);
>> visit_free(v);
>> return ret;
>> }
>>
>> Slightly shorter, but populating 'ret' at a distance feels a bit weird.
>
> I like not having to deal with the QmpOutputVisitor type, but like you,
> I don't like action at a distance.
>
>> Maybe we need to keep the FOO_new() functions returning FOO* rather
>> than Visitor*, along with the FOO_get_visitor() functions, after all.
>
> I can think of a other ways to hide the QmpOutputVisitor type, but they
> have drawbacks, too.
>
> You can let the two output functions take Visitor *. Drawback: now the
> compiler lets you pass the wrong kind of visitor.
But at least you can assert that the right visitor was used.
>
> You can let visit_complete() return the output (if any) as void *.
> Drawback: now the compiler lets you misinterpret the output's type.
And you lose any chance to assert things.
Another (hybrid?) option:
void visit_complete(Visitor *v, void *opaque);
Visitor *qmp_output_visitor_new(QObject **ret);
called as:
v = qmp_output_visitor_new(&ret);
...
visit_complete(v, &ret);
where the completion function asserts that opaque matches the same
pointer as passed in with correct type during new().
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
