On 11/12/2015 08:11 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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.

>> +++ 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org