From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51020) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzQRM-0003xy-Bj for qemu-devel@nongnu.org; Thu, 19 Nov 2015 09:44:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZzQRH-0006xv-CT for qemu-devel@nongnu.org; Thu, 19 Nov 2015 09:44:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43410) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzQRH-0006xp-59 for qemu-devel@nongnu.org; Thu, 19 Nov 2015 09:43:55 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id D50443B759 for ; Thu, 19 Nov 2015 14:43:54 +0000 (UTC) From: Markus Armbruster References: <1447836791-369-29-git-send-email-eblake@redhat.com> <1447890426-16318-1-git-send-email-eblake@redhat.com> <87egfme57h.fsf@blackfin.pond.sub.org> <564DDA5B.7070909@redhat.com> Date: Thu, 19 Nov 2015 15:43:52 +0100 In-Reply-To: <564DDA5B.7070909@redhat.com> (Eric Blake's message of "Thu, 19 Nov 2015 07:19:07 -0700") Message-ID: <87d1v63v87.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] fixup? qapi: Simplify QObject List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Luiz Capitulino Eric Blake writes: > On 11/19/2015 01:58 AM, Markus Armbruster wrote: >> Eric Blake 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.