On Mon, Mar 9, 2015 at 10:54 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote: > > > On Mon, Mar 9, 2015 at 7:48 PM, Connor Abbott <cwabbo...@gmail.com> wrote: >> >> On Mon, Mar 9, 2015 at 10:35 PM, Matt Turner <matts...@gmail.com> wrote: >> > On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner <matts...@gmail.com> wrote: >> >> On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke <kenn...@whitecape.org> >> >> wrote: >> >>> From: Jason Ekstrand <jason.ekstr...@intel.com> >> >>> >> >>> __next and __prev are pointers to the structure containing the >> >>> exec_node >> >>> link, not the embedded exec_node. NULL checks would fail unless the >> >>> embedded exec_node happened to be at offset 0 in the parent struct. >> >>> >> >>> Signed-off-by: Jason Ekstrand <jason.ekstr...@intel.com> >> >>> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> >> >>> --- >> >>> src/glsl/list.h | 4 ++-- >> >>> 1 file changed, 2 insertions(+), 2 deletions(-) >> >>> >> >>> diff --git a/src/glsl/list.h b/src/glsl/list.h >> >>> index ddb98f7..680e963 100644 >> >>> --- a/src/glsl/list.h >> >>> +++ b/src/glsl/list.h >> >>> @@ -684,7 +684,7 @@ inline void exec_node::insert_before(exec_list >> >>> *before) >> >>> exec_node_data(__type, (__list)->head, __field), >> >>> \ >> >>> * __next = >> >>> \ >> >>> exec_node_data(__type, (__node)->__field.next, __field); >> >>> \ >> >>> - __next != NULL; >> >>> \ >> >>> + &__next->__field != NULL; >> >>> \ >> >> >> >> I'm not understanding now the address of __next->__field can ever be >> >> NULL. >> >> >> >> __next is something with an embedded struct exec_node, so don't we >> >> want "__next->__field != NULL" without the address-of operator? >> > >> > Sorry, that should have been >> > "exec_node_is_tail_sentinel(&__next->__field)" >> >> No, that won't work. We want to bail out if the *current* node is the >> tail sentinel, not the next one. If the next node is the tail >> sentinel, then the current one is the last element, so we have to go >> through the loop once more. We could use exec_node_is_tail_sentinel() >> on the current node, > > > No, we can't. The whole point of the *_safe versions is that we never touch > the current node after we've let the client execute code. We stash off the > next one so that, even if the delete the current one, we still have > something to give them next iteration. > --Jason
Ah, right. My only concern is that doing pointer arithmetic on NULL isn't defined, and given that what we're doing involves underflowing the pointer so it wraps around to a large address (when subtracting in exec_node_get_data()) and then overflowing it back to 0 (when doing &__next->field), it's likely that some compiler might come along and "optimize" this. > >> >> but we've already dereferenced the pointer >> earlier when getting the next node so it would be less efficient to >> dereference it again. >> >> > _______________________________________________ >> > mesa-dev mailing list >> > mesa-dev@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev >> _______________________________________________ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev