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

[Eric Blake <eblake@redhat.com>]

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)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org