On 06.04.2012 17:38, nobled wrote:
> On Mon, Apr 2, 2012 at 7:40 AM, Tolga Dalman
> <tolga.dal...@googlemail.com> wrote:
> Hi,
>
>
> On 02.04.2012 00:24, nobled wrote:
>>>> #define LIST_FOR_EACH_ENTRY(pos, head, member)
>>>> \
>>>> - for (pos = container_of((head)->next, pos, member);
>>>> \
>>>> + for (pos = NULL, pos = container_of((head)->next, pos, member); \
>>>> &pos->member != (head); \
>>>> pos = container_of(pos->member.next, pos, member))
>
> The container_of macro is obviously error-prone and leads to future
> problems. Your patch works around that buggy API instead of
> really fixing it.
>
> container_of could be defined using the offsetof macro, i.e.,
>
> #define container_of(ptr, type, member) \
> (void *)((char *)(ptr) - offsetof(type, member))
>
> As far as I can see, container_of is only used sparsely in
> mesa, so applying my proposal should be possible quite easily.
>> Not really. container_of() is only used *directly* in this one header;
>> but changing it to require a type parameter requires also changing all
>> the macros that use it in turn, which actually are used widely in
>> mesa.
Widely is a bit exaggerated. Basically, it is only used in the
LIST_FOR_EACH macros. A quick grep results:
grep "[[:space:]]LIST_FOR_EACH" * -R|grep -v u_double_list.h|wc -l
35
I also noticed, that LIST_FOR_EACH_ENTRY_FROM and LIST_FOR_EACH_ENTRY_FROM_REV
are not used anywhere in the mesa source.
>> Adding the type name to every use of the LIST_FOR_EACH_* macros
>> also introduces redundancy and the possibility of error, e.g. if the
>> containing ever changes but not every use of the macro is updated,
>> there's no compile error but it'll go wrong at runtime. That can't
>> happen with the current version of it.
Easy: use typeof(), i.e., the LIST_FOR_EACH_* macros are updated as follows:
#define LIST_FOR_EACH_ENTRY(pos, head, member) \
for (pos = container_of((head)->next, typeof(pos), member); \
&pos->member != (head); \
pos = container_of(pos->member.next, typeof(pos), member))
In the long term, the container_of/LIST_FOR_EACH mess should go away.
Instead, just implement the clean linux kernel list_for_each
implementation (whence, I presume, the Mesa list is borrowed from).
>> container_of() just has one basic caveat that we can easily satisfy,
>> since like you said, it isn't directly used in many places, and I
>> don't think future problems are actually likely--any misuse will
>> trigger uninitialized warnings like the above so they're easily
>> caught, unlike possible typename mistakes.
Agreed.
Thanks and best regards
Tolga Dalman
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
