On Mon, Mar 9, 2015 at 7:59 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
> 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. > We could cast everything through uintptr_t but that's gonna get messy... > > > > >> > >> 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