On 11/19/2015 01:58 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> 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
