On Mon, 20 Aug 2012 15:05:25 -0700
Michel Lespinasse <[email protected]> wrote:
> In rb_erase, move the easy case (node to erase has no more than
> 1 child) first. I feel the code reads easier that way.
Well. For efficiency we should put the commonest case first. Is that
the case here?
> --- a/lib/rbtree.c
> +++ b/lib/rbtree.c
> @@ -368,17 +368,28 @@ static void __rb_erase_color(struct rb_node *node,
> struct rb_node *parent,
>
> void rb_erase(struct rb_node *node, struct rb_root *root)
> {
> - struct rb_node *child, *parent;
> + struct rb_node *child = node->rb_right, *tmp = node->rb_left;
Coding style nit: multiple-definitions-per-line makes it harder to
locate a particular definition, and from long experience I can assure
you that it makes management of subsequent overlapping patches quite a
lot harder. Also, one-definition-per-line gives room for a nice little
comment, and we all like nice little comments.
Also, "tmp" is a rotten name. Your choice of an identifier is your
opportunity to communicate something to the reader. When you choose
"tmp", you threw away that opportunity. Should it be called "left"?
--- a/lib/rbtree.c~rbtree-place-easiest-case-first-in-rb_erase-fix
+++ a/lib/rbtree.c
@@ -368,12 +368,13 @@ static void __rb_erase_color(struct rb_n
void rb_erase(struct rb_node *node, struct rb_root *root)
{
- struct rb_node *child = node->rb_right, *tmp = node->rb_left;
+ struct rb_node *child = node->rb_right
+ struct rb_node *tmp = node->rb_left;
struct rb_node *parent;
int color;
if (!tmp) {
- case1:
+case1:
/* Case 1: node to erase has no more than 1 child (easy!) */
parent = rb_parent(node);
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
