On Sun, Apr 8, 2012 at 4:28 AM, Tolga Dalman <tolga.dal...@googlemail.com> wrote: > 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: Love to, but typeof() is a GNU extension: http://gcc.gnu.org/onlinedocs/gcc/Typeof.html
Unless MSVC suddenly started supporting it, it's a no-go in gallium code (which is supposed to be portable so it has to stick to C89). > > #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
