Re: [Qemu-devel] [PATCH v11 09/24] qapi: Prefer typesafe upcasts to qapi base classes

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 10/27/2015 01:46 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> A previous patch (commit 1e6c1616) made it possible to
>>> directly cast from a qapi flat union type to its base type.
>>> However, it requires the use of a C cast, which turns off
>>> compiler type-safety checks.  Add inline type-safe wrappers
>>> named qapi_FOO_base() for any union type FOO that has a base,
>>> which can be used for a safer upcast, and enhance the
>>> testsuite to cover the new functionality.  A future patch
>>> will extend the upcast support to structs.
>>>
>>> Note that C makes const-correct upcasts annoying because
>>> it lacks overloads; these functions cast away const so that
>>> they can accept user pointers whether const or not, and the
>>> result in turn can be assigned to normal or const pointers.
>>> Alternatively, this could have been done with macros, but
>>> those tend to not have quite as much type safety.
>> 
>> Well, the macros can be made just as type-safe, but the result is either
>> somewhat ugly and using gcc-isms, or very ugly and unhygienic.
>> 
>> I'd write something like "type-safe macros are hairy, and not worthwhile
>> here."
>
> Sure, that works for me.

Sold.

>>> This patch just adds upcasts.  None of our code needed to
>>> downcast from a base qapi class to a child.
>> 
>> Actually, none of our code needs to upcast unions, either.  Only the new
>> tests do.  Code that updasts structs exist, but it doesn't use this
>> patch's upcasts until later.
>> 
>> Suggest to amend the first paragraph:
>> 
>>     A previous patch (commit 1e6c1616) made it possible to directly cast
>>     from a qapi flat union type to its base type.  However, it requires
>>     the use of a C cast, which turns off compiler type-safety checks.
>>     Fortunately, no such casts exist just, yet.
>
> s/ just,/, just/

Yes, thanks.

>> 
>>     Regardless, add inline type-safe wrappers named qapi_FOO_base() for
>>     any union type FOO that has a base, which can be used for a safer
>>     upcast, and enhance the testsuite to cover the new functionality.
>> 
>>     A future patch will extend the upcast support to structs, where such
>>     casts do exist already.
>> 
>
> Maybe s/casts/conversions/ - because as of this patch, it is still a
> conversion via foo->base rather than (Base *)foo (it's the next patch
> that gets rid of base, and therefore needs either the cast or the wrapper).

Sold.

>> Patch looks good.  I can touch up the commit message in my tree.
>
> Sure, your proposed wording + touchups is fine.