Re: [Qemu-devel] [PATCH v3 3/7] qapi: Replace qobject_to_X(o) by qobject_to(o, X)

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3800 bytes --]

On 2018-02-27 15:47, Eric Blake wrote:
> On 02/26/2018 06:01 AM, Max Reitz wrote:
> 
>>>> +++ b/block.c
>>>> @@ -1457,7 +1457,7 @@ static QDict *parse_json_filename(const char
>>>> *filename, Error **errp)
>>>>            return NULL;
>>>>        }
>>>>    -    options = qobject_to_qdict(options_obj);
>>>> +    options = qobject_to(options_obj, QDict);
>>>
>>> Bikeshedding - would it read any easier as:
>>>
>>> options = qobject_to(QDict, options_obj);
>>>
>>> ?  If so, your Coccinelle script can be touched up, and patch 2/7 swaps
>>> argument order around, so it would be tolerable but still slightly
>>> busywork to regenerate the series.  But I'm not strongly attached to
>>> either order, and so I'm also willing to take this as-is (especially
>>> since that's less work), if no one else has a strong opinion that
>>> swapping order would aid legibility.
>>
>> Well, same for me. :-)
>>
>> In a template/generic language, we'd write the type first (e.g.
>> qobject_cast<QDict>(options_obj)).  But maybe we'd write the object
>> first, too (e.g. options_obj.cast<QDict>()).  And the current order of
>> the arguments follows the order in the name ("qobject" options_obj "to"
>> QDict).  But maybe it's more natural to read it as "qobject to" QDict
>> "applied to" options_obj.
>>
>> I don't know either.
> 
> Okay, after looking for existing uses of type names in macro calls, I see:
> 
> qemu/compiler.h:
> 
> #ifndef container_of
> #define container_of(ptr, type, member) ({                      \
>         const typeof(((type *) 0)->member) *__mptr = (ptr);     \
>         (type *) ((char *) __mptr - offsetof(type, member));})
> #endif
> 
> /* Convert from a base type to a parent type, with compile time
> checking.  */
> #ifdef __GNUC__
> #define DO_UPCAST(type, field, dev) ( __extension__ ( { \
>     char __attribute__((unused)) offset_must_be_zero[ \
>         -offsetof(type, field)]; \
>     container_of(dev, type, field);}))
> #else
> #define DO_UPCAST(type, field, dev) container_of(dev, type, field)
> #endif
> 
> #define typeof_field(type, field) typeof(((type *)0)->field)
> 
> 
> qapi/clone-visitor.h:
> 
> /*
>  * Deep-clone QAPI object @src of the given @type, and return the result.
>  *
>  * Not usable on QAPI scalars (integers, strings, enums), nor on a
>  * QAPI object that references the 'any' type.  Safe when @src is NULL.
>  */
> #define QAPI_CLONE(type, src)                                           \
> 
> /*
>  * Copy deep clones of @type members from @src to @dst.
>  *
>  * Not usable on QAPI scalars (integers, strings, enums), nor on a
>  * QAPI object that references the 'any' type.
>  */
> #define QAPI_CLONE_MEMBERS(type, dst, src)                              \
> 
> 
> 2 out of 3 macros in compiler.h put the type name first, and
> container_of() puts it in the middle of 3.  It's even weirder because
> DO_UPCAST(t, f, d) calls container_of(d, t, f), where the inconsistency
> makes it a mental struggle to figure out how to read the two macros side
> by side, compared to if we had just been consistent.  Meanwhile, all of
> the macros in qapi put the type name first.
> 
> So at this point, I'm 70:30 in favor of doing the rename to have
> qobject_to(type, obj) for consistency with majority of other macros that
> take a type name (type names are already unusual as arguments to macros,
> whether or not the macro is named with ALL_CAPS).  (Sorry, I know that
> means more busy work for you, if you agree with my reasoning)

I agree, because it means I have a decision. :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]