From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51701) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwucF-00087r-7Z for qemu-devel@nongnu.org; Thu, 12 Nov 2015 11:20:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwucC-00039O-2C for qemu-devel@nongnu.org; Thu, 12 Nov 2015 11:20:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42981) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwucB-00039B-Mi for qemu-devel@nongnu.org; Thu, 12 Nov 2015 11:20:47 -0500 From: Markus Armbruster References: <1447224690-9743-1-git-send-email-eblake@redhat.com> <1447224690-9743-26-git-send-email-eblake@redhat.com> <877flnjlqr.fsf@blackfin.pond.sub.org> <5644B0A2.3020606@redhat.com> Date: Thu, 12 Nov 2015 17:20:44 +0100 In-Reply-To: <5644B0A2.3020606@redhat.com> (Eric Blake's message of "Thu, 12 Nov 2015 08:30:42 -0700") Message-ID: <87si4bfaub.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v11 25/28] qapi: Simplify visits of optional fields List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > On 11/12/2015 08:11 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> None of the visitor callbacks would set an error when testing >>> if an optional field was present; make this part of the interface >>> contract by eliminating the errp argument. Then, for less code, >>> reflect the determined boolean value back to the caller instead >>> of making the caller read the boolean after the fact. >>> >>> The resulting generated code has a nice diff: >>> >>> |- visit_optional(v, &has_fdset_id, "fdset-id", &err); >>> |- if (err) { >>> |- goto out; >>> |- } >>> |- if (has_fdset_id) { >>> |+ if (visit_optional(v, &has_fdset_id, "fdset-id")) { >>> | visit_type_int(v, &fdset_id, "fdset-id", &err); >>> | if (err) { >>> | goto out; >>> | } >>> | } >> >> Any particular reason not to do >> >> has_fdset_id = visit_optional(v, "fdset-id"); >> if (has_fdset_id) { > > We can't. Output visitors do not implement visit_optional() callbacks, > but must rely on the incoming value of has_fdset_id. Which means > assigning to has_fdset_id without an incoming value will do the wrong > thing. Or worded differently, &has_fdset_id is modified as an output > parameter by input visitors, and read unchanged as an input parameter by > output visitors. I guess I'd limit myself to just dropping the useless error checking then. Results in visit_optional(v, &has_fdset_id, "fdset-id") if (has_fdset_id) { which is slightly more verbose but also slightly more obvious. But it's all in generated code, so it doesn't really matter much either way. >>> +++ b/qapi/qapi-visit-core.c >>> @@ -73,12 +73,12 @@ void visit_end_union(Visitor *v, bool data_present, Error **errp) >>> } >>> } >>> >>> -void visit_optional(Visitor *v, bool *present, const char *name, >>> - Error **errp) >>> +bool visit_optional(Visitor *v, bool *present, const char *name) >>> { >>> if (v->optional) { >>> - v->optional(v, present, name, errp); >>> + v->optional(v, present, name); >>> } >>> + return *present; >>> } >> >> Slightly ugly: struct Visitor method optional returns void, but the >> wrapper returns bool. > > I could make all the callbacks (all 3 of them: opts-visitor, > qmp-input-visitor, string-input-visitor) return bool, but didn't see the > point in the churn, especially since the contract of the return value is > easy to do in the wrapper.