[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v11 25/28] qapi: Simplify visits of optional fie
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v11 25/28] qapi: Simplify visits of optional fields |
Date: |
Thu, 12 Nov 2015 17:20:44 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 11/12/2015 08:11 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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.
- [Qemu-devel] [PATCH v11 20/28] qapi: Forbid case-insensitive clashes, (continued)
- [Qemu-devel] [PATCH v11 20/28] qapi: Forbid case-insensitive clashes, Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 17/28] cpu: Convert CpuInfo into flat union, Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 24/28] qapi: Add positive tests to qapi-schema-test, Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 26/28] qapi: Move duplicate member checks to schema check(), Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 25/28] qapi: Simplify visits of optional fields, Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 22/28] qapi: Simplify visiting of alternate types, Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 28/28] qapi: Detect base class loops, Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 23/28] qapi: Fix alternates that accept 'number' but not 'int', Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 27/28] qapi: Move duplicate enum value checks to schema check(), Eric Blake, 2015/11/11