[Mesa-dev] [PATCH] glsl/list: make nodes safe for double removal, etc.

[Rob Clark]

On Tue, Apr 5, 2016 at 3:11 AM, Iago Toral <itoral at igalia.com> wrote:
> On Mon, 2016-04-04 at 11:04 -0400, Rob Clark wrote:
>> On Mon, Apr 4, 2016 at 10:34 AM, Iago Toral <itoral at igalia.com> wrote:
>> > On Sat, 2016-04-02 at 17:09 -0400, Rob Clark wrote:
>> >> From: Rob Clark <robclark at freedesktop.org>
>> >>
>> >> It's no extra overhead to do a _self_link() and it eliminates a class of
>> >> potential problems.
>> >
>> > it can also hide actual programming mistakes that would otherwise be
>> > immediately visible... does this actually help something specific?
>>
>> Well, basically it avoids needing to explicitly do a _self_link()
>> after removing a node in cases where you know (for example) that you
>> might end up removing multiple times.  The kernel list implementation
>> does have separate list_del() and list_del_init(), which would be a
>> different possible way to go.
>>
>> But in my experience the programming mistakes that this would hide are
>> simply cases where you wanted to do list_del_init() instead of
>> list_del(), so I'm curious about which other cases you are worried
>> about.
>
> I was thinking about incorrect double deletions of the same node. If we
> are never supposed to do that this would be hiding a programming
> mistake. In any case, I don't have a strong preference myself, I was
> just curious about the reasons for the change.

In most/all of the cases I can think of, I want a "remove this node if
it is still in a list", rather than keeping track if I had already
removed it.  Although a separate fxn equivalent to list_del_init() is
perhaps more sane if there are legitimate cases where double removal
is a mistake.

BR,
-R

>> Anyways, this patch doesn't solve something in particular, it is
>> mostly just a response to a comment Jason made about my usage of
>> immediate _self_link() after removal on another patch.
>
> I think having two functions for this, one that sets the pointers to
> NULL and one that self links is a better solution since you can choose
> the one that makes sense for what you are actually trying to do, but I
> don't have a strong preference, if others like this approach I am fine
> with it too.
>
> Iago
>
>> >
>> >> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>> >> Dared-by: Jason Ekstrand <jason at jlekstrand.net>
>> >> ---
>> >>  src/compiler/glsl/list.h | 15 +++++++--------
>> >>  1 file changed, 7 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/src/compiler/glsl/list.h b/src/compiler/glsl/list.h
>> >> index a1c4d82..77e1f67 100644
>> >> --- a/src/compiler/glsl/list.h
>> >> +++ b/src/compiler/glsl/list.h
>> >> @@ -165,19 +165,18 @@ exec_node_get_prev(struct exec_node *n)
>> >>  }
>> >>
>> >>  static inline void
>> >> -exec_node_remove(struct exec_node *n)
>> >> +exec_node_self_link(struct exec_node *n)
>> >>  {
>> >> -   n->next->prev = n->prev;
>> >> -   n->prev->next = n->next;
>> >> -   n->next = NULL;
>> >> -   n->prev = NULL;
>> >> +   n->next = n;
>> >> +   n->prev = n;
>> >>  }
>> >>
>> >>  static inline void
>> >> -exec_node_self_link(struct exec_node *n)
>> >> +exec_node_remove(struct exec_node *n)
>> >>  {
>> >> -   n->next = n;
>> >> -   n->prev = n;
>> >> +   n->next->prev = n->prev;
>> >> +   n->prev->next = n->next;
>> >> +   exec_node_self_link(n);
>> >>  }
>> >>
>> >>  static inline void
>> >
>> >
>>
>
>