Eric Blake <ebl...@redhat.com> writes:
> On 10/26/2015 01:33 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> writes:
>>
>>> On 10/23/2015 09:30 AM, Markus Armbruster wrote:
>>>> Eric Blake <ebl...@redhat.com> writes:
>>>>
>>>>> A previous patch (commit 1e6c1616) made it possible to
>>>>> directly cast from a qapi type to its base type. A future
>>>>> patch will do likewise for structs. However, it requires
>>>>> the client code to use a C cast, which turns off compiler
>>>>> type-safety checks if the client gets it wrong. So this
>>>>
>>>> Who's the client? Suggest to simply drop "if the client gets it wrong".
>>>>
>>>>> patch adds inline type-safe wrappers named qapi_FOO_base()
>>>>> for any type FOO that has a base, which can be used to
>>>>> upcast a qapi type to its base, then uses the new generated
>>>>> functions in places where we were already casting.
>>>>>
>
>>>
>>> Indeed, if I don't port gen_upcast() to types until patch 10/25, then
>>> this hunk also has to move to patch 10. That means no clients of the
>>> upcast macros in patch 9/25, _unless_ I add testsuite coverage. Which I
>>> probably ought to do.
>>>
>>>>> switch (event) {
>>>>> case QAPI_EVENT_VNC_CONNECTED:
>>>>> - qapi_event_send_vnc_connected(si, vs->info->base, &error_abort);
>>>>> + qapi_event_send_vnc_connected(si,
>>>>> qapi_VncClientInfo_base(vs->info),
>>>>> + &error_abort);
>>>>> break;
>>>>> case QAPI_EVENT_VNC_INITIALIZED:
>>>>> qapi_event_send_vnc_initialized(si, vs->info, &error_abort);
>>>>
>>>> Not a single cast to union base?
>>>
>>> Not that I could find. So I'll have to create one in the testsuite.
>>
>> I guess I'd introduce gen_upcast() only with its first real user,
>> i.e. when unboxing struct base. At that time, its obvious
>> implementation should work for both struct and union, even though we
>> actually use it only for struct. If you want to add a test case for
>> unions, go ahead. I'm not sure I'd bother, because once we unify code
>> generation for the two, testing them separately won't add much value
>> anymore.
>
> It sounds like I have two options for v11:
>
> 1. Keep 9/25 introducing gen_upcast(), just for union types, and
> including testsuite coverage. In 10/25, make use of the upcast functions
> to struct as part of making structs sane.
>
> 2. Swap the patch order: do 10/25 to alter struct layout first, using
> ugly casts; then implement 9/25 that adds gen_upcast() and fixes the
> ugly casts to instead use the new upcast functions.
>
> I can go either way, so do you have any preference?
I think I'd 3. Do 10/25 first, with gen_upcast() squashed in, and used
for casts to base. That gen_upcast() will just work for unions, too.
Least churn.
But any of the three options should work.
