Re: [Qemu-devel] [PATCH v11 25/28] qapi: Simplify visits of optional fields
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.
Re: [Qemu-devel] [PATCH v11 25/28] qapi: Simplify visits of optional fields
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.
>> +++ 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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v11 25/28] qapi: Simplify visits of optional fields
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) {
visit_type_int(v, &fdset_id, "fdset-id", &err);
if (err) {
goto out;
}
}
?
> Signed-off-by: Eric Blake
>
> ---
> v11 (no v10): no change
> v9: no change
> v8: no change
> v7: rebase to no member.c_name()
> v6: rebase onto earlier testsuite and gen_err_check() improvements
> ---
> include/qapi/visitor-impl.h | 5 ++---
> include/qapi/visitor.h | 10 --
> qapi/opts-visitor.c | 2 +-
> qapi/qapi-visit-core.c | 6 +++---
> qapi/qmp-input-visitor.c| 3 +--
> qapi/string-input-visitor.c | 3 +--
> scripts/qapi.py | 7 +--
> 7 files changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index ac6c17e..81d1aa0 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -44,9 +44,8 @@ struct Visitor
> void (*type_any)(Visitor *v, QObject **obj, const char *name,
> Error **errp);
>
> -/* May be NULL */
> -void (*optional)(Visitor *v, bool *present, const char *name,
> - Error **errp);
> +/* May be NULL; most useful for input visitors. */
> +void (*optional)(Visitor *v, bool *present, const char *name);
>
> void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error
> **errp);
> void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error
> **errp);
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 250e8e1..dd4f0b3 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -39,8 +39,14 @@ void visit_end_implicit_struct(Visitor *v, Error **errp);
> void visit_start_list(Visitor *v, const char *name, Error **errp);
> GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
> void visit_end_list(Visitor *v, Error **errp);
> -void visit_optional(Visitor *v, bool *present, const char *name,
> -Error **errp);
> +
> +/**
> + * Check if an optional member @name of an object needs visiting.
> + * For input visitors, set *@present according to whether the
> + * corresponding visit_type_*() needs calling; for other visitors,
> + * leave *@present unchanged. Return *@present for convenience.
> + */
> +bool visit_optional(Visitor *v, bool *present, const char *name);
>
> /**
> * Determine the qtype of the item @name in the current object visit.
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index cd10392..ef5fb8b 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -488,7 +488,7 @@ opts_type_size(Visitor *v, uint64_t *obj, const char
> *name, Error **errp)
>
>
> static void
> -opts_optional(Visitor *v, bool *present, const char *name, Error **errp)
> +opts_optional(Visitor *v, bool *present, const char *name)
> {
> OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 52c132a..daaa3af 100644
> --- a/qapi/qapi-visit-core.c
> +++ 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.
>
> void visit_get_next_type(Visitor *v, QTypeCode *type, bool promote_int,
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 4238faf..9f0ed69 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -304,8 +304,7 @@ static void qmp_input_type_any(Visitor *v, QObject **obj,
> const char *name,
> *obj = qobj;
> }
>
> -static void qmp_input_optional(Visitor *v, bool *present, const char *name,
> - Error **errp)
> +static void qmp_input_optional(Visitor *v, bool *present, cons
