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

[Eric Blake]

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.

> 
>> 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/

> 
>     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).


> 
> Patch looks good.  I can touch up the commit message in my tree.

Sure, your proposed wording + touchups is fine.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature