Eric Blake <ebl...@redhat.com> writes:
> 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.
Let's do it early.
>>> @@ -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.
Fewer casts is good, but symmetry is also good.
>>>
>>> /* 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).
Okay, I can buy this one.
>>> +++ 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.
Okay, I'm confused.
Consider BlockdevRef, defined as
{ 'alternate': 'BlockdevRef',
'data': { 'definition': 'BlockdevOptions',
'reference': 'str' } }
where BlockdevOptions is a (flat) union. Let's clone a BlockdevRef
holding a str. Sequence of calls:
qapi_BlockdevRef_clone(src)
qapi_clone_visitor_new(src)
// qcv->depth is now 0
visit_type_BlockdevRef(v, NULL, &dst, &error_abort)
visit_start_alternate(v, NULL, &dst,
sizeof(BlockdevRef), true, &error_abort)
qapi_clone_start_alternate(v, NULL, &dst,
sizeof(BlockdevRef), true, &error_abort)
qapi_clone_start_struct(v, NULL, &dst,
sizeof(BlockdevRef), &error_abort)
// does not increment qcv->depth
visit_type_str(v, NULL, &dst->u.references, &error_abort)
qapi_clone_type_str(v, NULL, &dst->u.references, &error_abort)
assert(qcv->depth) // why does this hold?
>>> +++ 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.
