From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43159) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqlyE-00048K-8M for qemu-devel@nongnu.org; Mon, 26 Oct 2015 13:54:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZqlyB-0005kA-2I for qemu-devel@nongnu.org; Mon, 26 Oct 2015 13:54:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59345) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqlyA-0005k6-RI for qemu-devel@nongnu.org; Mon, 26 Oct 2015 13:54:06 -0400 From: Markus Armbruster References: <1445576998-2921-1-git-send-email-eblake@redhat.com> <1445576998-2921-10-git-send-email-eblake@redhat.com> <87lhatbo1o.fsf@blackfin.pond.sub.org> <562A9C46.8010909@redhat.com> <87r3ki2iev.fsf@blackfin.pond.sub.org> <562E53D8.4050205@redhat.com> Date: Mon, 26 Oct 2015 18:54:03 +0100 In-Reply-To: <562E53D8.4050205@redhat.com> (Eric Blake's message of "Mon, 26 Oct 2015 10:24:56 -0600") Message-ID: <871tchplc4.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v10 09/25] qapi: Prefer typesafe upcasts to qapi base classes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Gerd Hoffmann , Michael Roth Eric Blake writes: > On 10/26/2015 01:33 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> On 10/23/2015 09:30 AM, Markus Armbruster wrote: >>>> Eric Blake writes: >>>> >>>>> A previous patch (commit 1e6c1616) made it possible to >>>>> directly cast from a qapi type to its base type. A future >>>>> patch will do likewise for structs. However, it requires >>>>> the client code to use a C cast, which turns off compiler >>>>> type-safety checks if the client gets it wrong. So this >>>> >>>> Who's the client? Suggest to simply drop "if the client gets it wrong". >>>> >>>>> patch adds inline type-safe wrappers named qapi_FOO_base() >>>>> for any type FOO that has a base, which can be used to >>>>> upcast a qapi type to its base, then uses the new generated >>>>> functions in places where we were already casting. >>>>> > >>> >>> Indeed, if I don't port gen_upcast() to types until patch 10/25, then >>> this hunk also has to move to patch 10. That means no clients of the >>> upcast macros in patch 9/25, _unless_ I add testsuite coverage. Which I >>> probably ought to do. >>> >>>>> switch (event) { >>>>> case QAPI_EVENT_VNC_CONNECTED: >>>>> - qapi_event_send_vnc_connected(si, vs->info->base, &error_abort); >>>>> + qapi_event_send_vnc_connected(si, qapi_VncClientInfo_base(vs->info), >>>>> + &error_abort); >>>>> break; >>>>> case QAPI_EVENT_VNC_INITIALIZED: >>>>> qapi_event_send_vnc_initialized(si, vs->info, &error_abort); >>>> >>>> Not a single cast to union base? >>> >>> Not that I could find. So I'll have to create one in the testsuite. >> >> I guess I'd introduce gen_upcast() only with its first real user, >> i.e. when unboxing struct base. At that time, its obvious >> implementation should work for both struct and union, even though we >> actually use it only for struct. If you want to add a test case for >> unions, go ahead. I'm not sure I'd bother, because once we unify code >> generation for the two, testing them separately won't add much value >> anymore. > > It sounds like I have two options for v11: > > 1. Keep 9/25 introducing gen_upcast(), just for union types, and > including testsuite coverage. In 10/25, make use of the upcast functions > to struct as part of making structs sane. > > 2. Swap the patch order: do 10/25 to alter struct layout first, using > ugly casts; then implement 9/25 that adds gen_upcast() and fixes the > ugly casts to instead use the new upcast functions. > > I can go either way, so do you have any preference? I think I'd 3. Do 10/25 first, with gen_upcast() squashed in, and used for casts to base. That gen_upcast() will just work for unions, too. Least churn. But any of the three options should work.