[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 36/37] RFC: qapi: Adjust layout of FooList ty
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v9 36/37] RFC: qapi: Adjust layout of FooList types |
Date: |
Fri, 29 Jan 2016 09:19:59 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 01/28/2016 08:34 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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!
- Re: [Qemu-devel] [PATCH v9 16/37] qapi: Swap 'name' in visit_* callbacks to match public API, (continued)
[Qemu-devel] [PATCH v9 32/37] qapi: Rework deallocation of partial struct, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 33/37] qapi: Split visit_end_struct() into pieces, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 36/37] RFC: qapi: Adjust layout of FooList types, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 35/37] qapi: Change visit_type_FOO() to no longer return partial objects, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 34/37] qapi: Simplify semantics of visit_next_list(), Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 15/37] qom: Swap 'name' next to visitor in ObjectPropertyAccessor, Eric Blake, 2016/01/19