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
signature.asc
Description: OpenPGP digital signature
