Eric Blake <ebl...@redhat.com> writes:
> Making each visitor provide its own (awkwardly-named) FOO_cleanup()
> is unusual, when we can instead have a polymorphic visit_free()
> interface.
>
> The dealloc visitor is the first one converted to completely use
> the new entry point, since only generated code and the testsuite
> were using it. Diffs to the generated code look like:
>
> | void qapi_free_ACPIOSTInfo(ACPIOSTInfo *obj)
> | {
> |- QapiDeallocVisitor *qdv;
> | Visitor *v;
> |
> | if (!obj) {
> | return;
> | }
> |
> |- qdv = qapi_dealloc_visitor_new();
> |- v = qapi_dealloc_get_visitor(qdv);
> |+ v = qapi_dealloc_visitor_new();
At this point, I wonder what this change has to do with the new
visit_free(). It turns out that...
> | visit_type_ACPIOSTInfo(v, NULL, &obj, NULL);
> |- qapi_dealloc_visitor_cleanup(qdv);
... this call is the only use of qdv, so you're eliminating it.
Next, I wonder whether this is worthwhile, as it creates an
inconsistency with the other visitors. Peeking ahead, I see the
inconsistency is temporary: you're eliminating the other _get_visitor()
later in this series. Okay, but perhaps you can explain your intentions
further up in the commit message.
> |+ visit_free(v);
> |}
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
[...]
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index dcfbf92..28d2203 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -513,6 +513,14 @@ opts_optional(Visitor *v, const char *name, bool
> *present)
> }
>
>
> +static void
> +opts_free(Visitor *v)
> +{
> + OptsVisitor *ov = to_ov(v);
> + opts_visitor_cleanup(ov);
I'd eliminate the variable:
opts_visitor_cleanup(to_ov(v));
Hmm, you put it to use when you inline opts_visitor_cleanup() in the
next patch. Doesn't matter then.
> +}
> +
> +
> OptsVisitor *
> opts_visitor_new(const QemuOpts *opts)
> {
> @@ -540,6 +548,7 @@ opts_visitor_new(const QemuOpts *opts)
> * skip some mandatory methods... */
>
> ov->visitor.optional = &opts_optional;
> + ov->visitor.free = opts_free;
>
> ov->opts_root = opts;
>
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 9391dea..235e8a1 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -107,17 +107,12 @@ static void qapi_dealloc_type_null(Visitor *v, const
> char *name, Error **errp)
> {
> }
>
> -Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v)
> -{
> - return &v->visitor;
> -}
> -
> -void qapi_dealloc_visitor_cleanup(QapiDeallocVisitor *v)
> +static void qapi_dealloc_free(Visitor *v)
> {
> g_free(v);
Uh, shouldn't this be g_free(v, QapiDeallocVisitor, visitor)? That way,
we don't assume that visitor is QapiDeallocVisitor's first member.
> }
>
> -QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
> +Visitor *qapi_dealloc_visitor_new(void)
> {
> QapiDeallocVisitor *v;
>
> @@ -138,6 +133,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
> v->visitor.type_number = qapi_dealloc_type_number;
> v->visitor.type_any = qapi_dealloc_type_anything;
> v->visitor.type_null = qapi_dealloc_type_null;
> + v->visitor.free = qapi_dealloc_free;
>
> - return v;
> + return &v->visitor;
> }
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 84f32fc..3ca192e 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -375,6 +375,12 @@ Visitor *qmp_input_get_visitor(QmpInputVisitor *v)
> return &v->visitor;
> }
>
> +static void qmp_input_free(Visitor *v)
> +{
> + QmpInputVisitor *qiv = to_qiv(v);
> + qmp_input_visitor_cleanup(qiv);
> +}
> +
Like for opts_free(), the variable becomes useful when
qmp_input_visitor_cleanup() gets inlined in a later patch.
Same for the other free methods.
> void qmp_input_visitor_cleanup(QmpInputVisitor *v)
> {
> qobject_decref(v->root);
> @@ -403,6 +409,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj, bool
> strict)
> v->visitor.type_any = qmp_input_type_any;
> v->visitor.type_null = qmp_input_type_null;
> v->visitor.optional = qmp_input_optional;
> + v->visitor.free = qmp_input_free;
> v->strict = strict;
>
> v->root = obj;
[...]
