Re: [Xen-devel] [PATCH v2 05/20] rb_tree: remove redundant if()-condition in rb_erase()

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Sat, 2017-06-17 at 15:02 +0530, Praveen Kumar wrote:
> Furthermore, notice that the initial checks:
> 
>             if (!node->rb_left)
>                     child = node->rb_right;
>             else if (!node->rb_right)
>                     child = node->rb_left;
>             else
>             {
>                     ...
>             }
> guarantee that old->rb_right is set in the final else branch,
> therefore
> we can omit checking that again.
> 
> Signed-off-by: Wolfram Strepp <wstrepp@xxxxxx>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> [Linux commit 4b324126e0c6c3a5080ca3ec0981e8766ed6f1ee]
> 
And yet, the actual patch is slightly different. As in...

> --- a/xen/common/rbtree.c
> +++ b/xen/common/rbtree.c
> @@ -250,15 +250,16 @@ void rb_erase(struct rb_node *node, struct
> rb_root *root)
>              if (child)
>                  rb_set_parent(child, parent);
>              parent->rb_left = child;
> +
> +            node->rb_right = old->rb_right;
> +            rb_set_parent(old->rb_right, node);
>          }
>  
>          node->rb_parent_color = old->rb_parent_color;
> -        node->rb_right = old->rb_right;
>          node->rb_left = old->rb_left;
>  
...In the Linux commit, this blank line is removed too.

>          rb_set_parent(old->rb_left, node);
> -        if (old->rb_right)
> -            rb_set_parent(old->rb_right, node);
> +
>          goto color;
>      }
>  
I don't think this is too big of a deal per se, I'd I'd leave to
maintainers and committers to decide whether something like this is
enough for asking a resend, or whether it can be fixed upon commit or
even left as it is.

For sure, we know that these patches really needs to be, as much as
possible, 1:1 copies of Linux's ones, even in the smallest detail. And
the reason is to make the life of someone wanting to do another round
of import, in future, as easy as possible.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel