[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v3 3/7] qapi: Replace qobject_to_X(o) by qobject

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v3 3/7] qapi: Replace qobject_to_X(o) by qobject_to(o, X)
Date: Tue, 27 Feb 2018 08:47:20 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

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:


#ifndef container_of
#define container_of(ptr, type, member) ({                      \
        const typeof(((type *) 0)->member) *__mptr = (ptr);     \
        (type *) ((char *) __mptr - offsetof(type, member));})

/* 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);}))
#define DO_UPCAST(type, field, dev) container_of(dev, type, field)

#define typeof_field(type, field) typeof(((type *)0)->field)


 * 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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]