On 05/02/2016 12:20 PM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> Rather than making the dealloc visitor track of stack of pointers >> remembered during visit_start_* in order to free them during >> visit_end_*, it's a lot easier to just make all callers pass the >> same pointer to visit_end_*. The generated code has access to the >> same pointer, while all other users are doing virtual walks and >> can pass NULL. The dealloc visitor is then greatly simplified. >> >> The clone visitor also gets a minor simplification of not having >> to track quite as much depth. > > Do this before adding the clone visitor, to reduce churn?
I could. I first thought of it after, and kept it there as a
justification for keeping it (multiple visitors benefit), but even if
rebased earlier, the commit message can still mention that it will make
the clone visitor easier.
>> @@ -59,7 +59,7 @@ struct Visitor
>> GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
>>
>> /* Must be set */
>> - void (*end_list)(Visitor *v);
>> + void (*end_list)(Visitor *v, void **list);
>
> Sure you want void ** and not GenericList **?
Yes. There are only two callers: dtc code passes NULL (where the type
doesn't matter), and generated visit_type_FOOList() has to already cast
to GenericList() during visit_start_list(); accepting void** here
instead of GenericList** removes the need for a second instance of that
cast.
>
>>
>> /* Must be set by input and dealloc visitors to visit alternates;
>> * optional for output visitors. */
>> @@ -68,7 +68,7 @@ struct Visitor
>> bool promote_int, Error **errp);
>>
>> /* Optional, needed for dealloc visitor */
>> - void (*end_alternate)(Visitor *v);
>> + void (*end_alternate)(Visitor *v, void **obj);
>
> Sure you want void ** and not GenericAlternate **?
Only one caller: generated code. Same story that we already have to cast
during visit_start_alternate(), so using void** here avoids the need for
a second cast.
Oh, and the clone visitor was easier to write with a single function
that takes void** for all three visit_end() implementations (whereas I'd
have to write three functions identical except for signature otherwise).
>> +++ b/qapi/qapi-clone-visitor.c
>> @@ -29,11 +29,8 @@ static void qapi_clone_start_struct(Visitor *v, const
>> char *name, void **obj,
>> QapiCloneVisitor *qcv = to_qcv(v);
>>
>> if (!obj) {
>> - /* Nothing to allocate on the virtual walk during an
>> - * alternate, but we still have to push depth.
>> - * FIXME: passing obj to visit_end_struct would make this easier */
>> + /* Nothing to allocate on the virtual walk */
>> assert(qcv->depth);
>> - qcv->depth++;
>> return;
>> }
>>
>
> Why can we elide qcv->depth++? Do the assert(qcv->qdepth) still hold?
Yes, the assertion still holds: we can only reach this code underneath
visit_start_alternate(), ...
>
>> @@ -41,11 +38,13 @@ static void qapi_clone_start_struct(Visitor *v, const
>> char *name, void **obj,
>> qcv->depth++;
>> }
>>
>> -static void qapi_clone_end(Visitor *v)
>> +static void qapi_clone_end(Visitor *v, void **obj)
>> {
>> QapiCloneVisitor *qcv = to_qcv(v);
>> assert(qcv->depth);
>> - qcv->depth--;
>> + if (obj) {
>> + qcv->depth--;
>> + }
...and this is the matching elision of the depth manipulations.
>> +++ b/qapi/qmp-input-visitor.c
>> @@ -145,7 +145,7 @@ static void qmp_input_check_struct(Visitor *v, Error
>> **errp)
>> }
>> }
>>
>> -static void qmp_input_pop(Visitor *v)
>> +static void qmp_input_pop(Visitor *v, void **obj)
>> {
>> QmpInputVisitor *qiv = to_qiv(v);
>> StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
>
> You could assert @obj matches tos->obj. Same for the other visitors
> that still need a stack.
And brings me back to my question of whether qapi-visit-core.c should
maintain its own stack for asserting these types of sanity checks for
ALL callers, even when the visitor itself doesn't need a stack.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
