From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48938) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aP4Hn-0002j7-OQ for qemu-devel@nongnu.org; Fri, 29 Jan 2016 03:20:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aP4Hj-0007qg-NW for qemu-devel@nongnu.org; Fri, 29 Jan 2016 03:20:07 -0500 From: Markus Armbruster References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-37-git-send-email-eblake@redhat.com> <878u39iuqf.fsf@blackfin.pond.sub.org> <56AA4E8F.2030808@redhat.com> Date: Fri, 29 Jan 2016 09:19:59 +0100 In-Reply-To: <56AA4E8F.2030808@redhat.com> (Eric Blake's message of "Thu, 28 Jan 2016 10:23:27 -0700") Message-ID: <877fisdcgg.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v9 36/37] RFC: qapi: Adjust layout of FooList types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Alexander Graf , qemu-devel@nongnu.org, Michael Roth , "open list:sPAPR" , marcandre.lureau@redhat.com, David Gibson Eric Blake writes: > On 01/28/2016 08:34 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> By sticking the next pointer first, we don't need a union with >>> 64-bit padding for smaller types. On 32-bit platforms, this >>> can reduce the size of uint8List from 16 bytes (or 12, depending >>> on whether 64-bit ints can tolerate 4-byte alignment) down to 8. >>> It has no effect on 64-bit platforms (where alignment still >>> dictates a 16-byte struct); but fewer anonymous unions is still >>> a win in my book. >>> >>> However, this requires visit_start_list() and visit_next_list() >>> to gain a size parameter, to know what size element to allocate. >>> >>> I debated about going one step further, to allow for fewer casts, >>> by doing: >>> typedef GenericList GenericList; >>> struct GenericList { >>> GenericList *next; >>> }; >>> struct FooList { >>> GenericList base; >>> Foo value; >>> }; >>> so that you convert to 'GenericList *' by '&foolist->base', and >>> back by 'container_of(generic, GenericList, base)' (as opposed to >>> the existing '(GenericList *)foolist' and '(FooList *)generic'). >>> But doing that would require hoisting the declaration of >>> GenericList prior to inclusion of qapi-types.h, rather than its >>> current spot in visitor.h; it also makes iteration a bit more >>> verbose through 'foolist->base.next' instead of 'foolist->next'. > > Should I attempt this? A quick grep for '(GenericList' finds two in qapi-code-gen.txt, and two in qapi-visit-py. I doubt avoiding them is worth much of your time or mine :) >>> typedef struct GenericList >>> { >>> - union { >>> - void *value; >>> - uint64_t padding; >>> - }; >>> struct GenericList *next; >>> + char padding[]; >>> } GenericList; >> >> Less trickery, I like it. >> >> Member padding appears to be unused. > > or just leave it at this? I'd say good enough. >>> bool visit_start_list(Visitor *v, const char *name, GenericList **list, >>> - Error **errp) >>> + size_t size, Error **errp) >>> { >>> - bool result = v->start_list(v, name, list, errp); >>> + bool result; >>> + >>> + assert(list ? size : !size); >> >> Tighter than size != 0 would be size >= GenericList. Same elsewhere. > > Makes sense. > >> >> Rest looks good. Didn't look as closely as for the previous patches >> (getting tired), but so far I like the idea. > > Okay, I'll keep it and drop the RFC. Thanks!