Eric Blake <ebl...@redhat.com> writes:
> 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.
I'll push my current branch shortly. I'm happy with it up to PATCH 26.
I'm still pondering PATCH 27.
>>> +++ 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?
Almost.
> 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.
In qobject.h:
/**
* qobject_decref(): Decrement QObject's reference count, deallocate
* when it reaches zero
*/
static inline void qobject_decref(QObject *obj)
{
assert(!obj || obj->refcnt);
if (obj && --obj->refcnt == 0) {
qobject_destroy(obj);
}
}
In qobject.c:
void qobject_destroy(QObject *obj)
{
assert(!obj->refcnt);
assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
qdestroy[obj->type](obj);
}
>>> +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.
