[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] fixup? qapi: Simplify QObject
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH] fixup? qapi: Simplify QObject |
Date: |
Thu, 19 Nov 2015 07:19:07 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 11/19/2015 01:58 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>
>> [If we squash, replace the old commit message with this; you'll
>> also have some merge conflicts in 29/36 and 30/36. Note that
>> while this mail is net lines added, squashing would still result
>> in a net reduction:
>> 16 files changed, 67 insertions(+), 83 deletions(-)
>> ]
I'm leaning towards squashing, so I'll post a v12.5 of 28-30.
>> +++ b/include/qapi/qmp/qobject.h
>> @@ -52,15 +52,17 @@ typedef struct QObject QObject;
>> struct QObject {
>> qtype_code type;
>> size_t refcnt;
>> - void (*destroy)(QObject *);
>> };
>>
>> +typedef void (*QDestroy)(QObject *);
>> +extern QDestroy qdestroy[QTYPE_MAX];
>> +
So if I'm understanding you, instead of declaring the table,...
>> @@ -97,8 +97,8 @@ static inline void qobject_decref(QObject *obj)
>> {
>> assert(!obj || obj->refcnt);
>> if (obj && --obj->refcnt == 0) {
>> - assert(obj->destroy);
>> - obj->destroy(obj);
>> + assert(qdestroy[obj->type]);
>> + qdestroy[obj->type](obj);
>> }
>> }
>>
>
> Preexisting: the assertion is of little value, because all it does is
> convert one kind of crash into another.
>
> Asserting obj->type is in range might be more useful, i.e.
>
> - assert(obj->destroy);
> - obj->destroy(obj);
> + assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
> + qdestroy[obj->type](obj);
>
> We could outline the conditional part, and keep qdestroy[] static.
> Could save a bit of code (unless NDEBUG, but we shouldn't optimize for
> that).
...I could do:
/* High-level interface for qobject_decref() */
#define QDECREF(obj) \
do { \
if (obj) { \
assert(QOBJECT(obj)->refcnt)); \
if (!--QOBJECT(obj)->refcnt) { \
qobject_decref(QOBJECT(obj)); \
} \
} \
} while (0)
and move qobject_decref() into qobject.c where the destroy table is then
static to that file.
Correct?
Oh wait, it won't work quite like that. We have direct callers of
qobject_decref() (QDECREF is only usable on subclasses, but if you have
a QObject* you can't use QDECREF), so I can't change semantics by
hoisting things into the macro.
>> +QDestroy qdestroy[QTYPE_MAX] = {
>> + [QTYPE_QBOOL] = qbool_destroy_obj,
>> + [QTYPE_QDICT] = qdict_destroy_obj,
>> + [QTYPE_QFLOAT] = qfloat_destroy_obj,
>> + [QTYPE_QINT] = qint_destroy_obj,
>> + [QTYPE_QLIST] = qlist_destroy_obj,
>> + [QTYPE_QSTRING] = qstring_destroy_obj,
>> + /* [QTYPE_QNULL] = NULL, */
>> +};
>
> Suggest
>
> QDestroy qdestroy[QTYPE_MAX] = {
> [QTYPE_QNULL] = NULL, /* no such object exists */
> [QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
Makes sense (with the obvious fix for QTYPE_NONE).
> If you'd like to respin, I'd prefer a non-incremental [PATCH v12.5
> 28/36] replacing v12.
Plus 29 and 30 for conflict resolutions.
>
> If not, I'm happy to squash in trivial changes like the one I suggested
> for qdestroy[]'s initializer.
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v12 24/36] cpu: Convert CpuInfo into flat union, (continued)
[Qemu-devel] [PATCH v12 20/36] blkdebug: Avoid '.' in enum values, Eric Blake, 2015/11/18
[Qemu-devel] [PATCH v12 28/36] qapi: Simplify QObject, Eric Blake, 2015/11/18
[Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes, Eric Blake, 2015/11/18
Re: [Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes, Eric Blake, 2015/11/18
[Qemu-devel] [PATCH] fixup! qapi: Forbid case-insensitive clashes, Eric Blake, 2015/11/18
Re: [Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes, Markus Armbruster, 2015/11/19