Re: [Qemu-devel] [PATCH v2 1/6] qapi: Add qobject_to()

[Max Reitz <mreitz@redhat.com>]

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

On 2018-01-22 16:33, Eric Blake wrote:
> On 01/20/2018 09:44 AM, Max Reitz wrote:
>> This is a dynamic casting macro that, given a QObject type, returns an
>> object as that type or NULL if the object is of a different type (or
>> NULL itself).
>>
>> The macro uses lower-case letters because:
>> 1. There does not seem to be a hard rule on whether qemu macros have to
>>    be upper-cased,
>> 2. The current situation in qapi/qmp is inconsistent (compare e.g.
>>    QINCREF() vs. qdict_put()),
>> 3. qobject_to() will evaluate its @obj parameter only once, thus it is
>>    generally not important to the caller whether it is a macro or not,
>> 4. I prefer it aesthetically.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> You're more than welcome to convince me to call it QOBJECT_TO()!
> 
> I can live with the lower-case version; especially with your good list
> of reasons for it ;)
> 
>> ---
>>  include/qapi/qmp/qobject.h | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
>> index 38ac68845c..1211989ca0 100644
>> --- a/include/qapi/qmp/qobject.h
>> +++ b/include/qapi/qmp/qobject.h
>> @@ -50,6 +50,24 @@ struct QObject {
>>  #define QDECREF(obj)              \
>>      qobject_decref(obj ? QOBJECT(obj) : NULL)
>>  
>> +/* Required for qobject_to() */
>> +#define QTYPE_CAST_TO_QNull     QTYPE_QNULL
>> +#define QTYPE_CAST_TO_QNum      QTYPE_QNUM
>> +#define QTYPE_CAST_TO_QString   QTYPE_QSTRING
>> +#define QTYPE_CAST_TO_QDict     QTYPE_QDICT
>> +#define QTYPE_CAST_TO_QList     QTYPE_QLIST
>> +#define QTYPE_CAST_TO_QBool     QTYPE_QBOOL
>> +
>> +#ifdef CONFIG_STATIC_ASSERT
>> +_Static_assert(QTYPE__MAX == 7,
>> +               "The QTYPE_CAST_TO_* list needs to be extended");
>> +#endif
> 
> Why not use QEMU_BUILD_BUG_ON() instead of _Static_assert?  That way,
> this check will break the build even on older compilers that lack
> _Static_assert but where we still know how to trigger a build failure.

One reason would be because _Static_assert() allows me to give a nice
warning message -- I could add a comment above the QEMU_BUILD_BUG_ON()
line and hope for the best, but still, I like that better.

The other is that I assumed most people would be using C11 compilers
now, so the omission would be spotted sufficiently early.
"Sufficiently" because it's actually not that bad for this list to be
incomplete.  The worst is that you can't use qobject_to() for your
desired target type, but that's hardly catastrophic.

I could add an #else branch with QEMU_BUILD_BUG_ON().  Or I could add a
QEMU_BUILD_BUG_MSG() macro that takes a message (which is omitted if you
don't have _Static_assert() but would still make for a nice comment).  I
guess the latter would be a nice idea in general...?

Max

>> +
>> +#define qobject_to(obj, type) \
>> +    container_of(qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)) ?: \
>> +                     QOBJECT((type *)NULL), \
>> +                 type, base)
> 
> Slick!
> 



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