From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45830) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzQ3V-00080J-DR for qemu-devel@nongnu.org; Thu, 19 Nov 2015 09:19:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZzQ3R-0000mN-7O for qemu-devel@nongnu.org; Thu, 19 Nov 2015 09:19:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46036) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzQ3R-0000mJ-0N for qemu-devel@nongnu.org; Thu, 19 Nov 2015 09:19:17 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 6E73491FD3 for ; Thu, 19 Nov 2015 14:19:16 +0000 (UTC) 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> From: Eric Blake Message-ID: <564DDA5B.7070909@redhat.com> Date: Thu, 19 Nov 2015 07:19:07 -0700 MIME-Version: 1.0 In-Reply-To: <87egfme57h.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="TGoegDJqBko1bWMX7kSXhjfjM03NNKUgx" Subject: Re: [Qemu-devel] [PATCH] fixup? qapi: Simplify QObject List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Luiz Capitulino This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --TGoegDJqBko1bWMX7kSXhjfjM03NNKUgx Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/19/2015 01:58 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> [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 =3D=3D 0) { >> - assert(obj->destroy); >> - obj->destroy(obj); >> + assert(qdestroy[obj->type]); >> + qdestroy[obj->type](obj); >> } >> } >> >=20 > Preexisting: the assertion is of little value, because all it does is > convert one kind of crash into another. >=20 > Asserting obj->type is in range might be more useful, i.e. >=20 > - assert(obj->destroy); > - obj->destroy(obj); > + assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX); > + qdestroy[obj->type](obj); >=20 > 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). =2E..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] =3D { >> + [QTYPE_QBOOL] =3D qbool_destroy_obj, >> + [QTYPE_QDICT] =3D qdict_destroy_obj, >> + [QTYPE_QFLOAT] =3D qfloat_destroy_obj, >> + [QTYPE_QINT] =3D qint_destroy_obj, >> + [QTYPE_QLIST] =3D qlist_destroy_obj, >> + [QTYPE_QSTRING] =3D qstring_destroy_obj, >> + /* [QTYPE_QNULL] =3D NULL, */ >> +}; >=20 > Suggest >=20 > QDestroy qdestroy[QTYPE_MAX] =3D { > [QTYPE_QNULL] =3D NULL, /* no such object exists */ > [QTYPE_QNULL] =3D 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. >=20 > If not, I'm happy to squash in trivial changes like the one I suggested= > for qdestroy[]'s initializer. >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --TGoegDJqBko1bWMX7kSXhjfjM03NNKUgx Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWTdpbAAoJEKeha0olJ0NqY8oH/1xzKztK0zJjyqz3+KRikYQr 0iaHn90AHS6MEGpvBzrl+r3Hbuo1O3M5im+hVy3rtEML5m6xcVQRgONEFdFA5XIN kdiciwKu0t736YaZabGV98LpcfuyXWaQpE8KXw+Punxs3W58iE4JF15Yqxh0knXH JmbXSjLoD1Ck0A7Vgxpqmq/5RDfx2F2ZFqrae6fcuwpaepN29/+eDGU10cZOKM+T Fa8sSRt6zJEcOur2bmC0iDcTBVCzOtyyClwuyrZHGGcYyyIrv71XTFhGZGxOAeia cKcujuDbdZrpDjPSCeuMmZd+mSZtOPHz0y+loI6xX5XGrxZYA5ScfToiFp2uspE= =xfxu -----END PGP SIGNATURE----- --TGoegDJqBko1bWMX7kSXhjfjM03NNKUgx--