Clarify the new Red-Black post-order traversal code a bit.

Coverity complained about the for(;;) loop, because it never actually
iterated. It was used just to be able to use "break" to exit it early. I
agree with Coverity, that's a bit confusing, so refactor the code to
use if-else instead.

While we're at it, use a local variable to hold the "current" node. That's
shorter and clearer than referring to "iter->last_visited" all the time.

diff --git a/src/backend/lib/rbtree.c b/src/backend/lib/rbtree.c
index 242e9803ccc54245046474671c174a3bb0e7937b..7fb7e55f716b24116dd99b4992aab5597479d1b4 100644 (file)
--- a/src/backend/lib/rbtree.c
+++ b/src/backend/lib/rbtree.c
@@ -781,27 +781,30 @@ static RBNode *
 rb_inverted_iterator(RBTreeIterator *iter)
 {
    RBNode     *came_from;
+   RBNode     *current;
+
+   current = iter->last_visited;
 
 loop:
    switch ((InvertedWalkNextStep) iter->next_step)
    {
            /* First call, begin from root */
        case NextStepBegin:
-           iter->last_visited = iter->rb->root;
+           current = iter->rb->root;
            iter->next_step = NextStepLeft;
            goto loop;
 
        case NextStepLeft:
-           while (iter->last_visited->left != RBNIL)
-               iter->last_visited = iter->last_visited->left;
+           while (current->left != RBNIL)
+               current = current->left;
 
            iter->next_step = NextStepRight;
            goto loop;
 
        case NextStepRight:
-           if (iter->last_visited->right != RBNIL)
+           if (current->right != RBNIL)
            {
-               iter->last_visited = iter->last_visited->right;
+               current = current->right;
                iter->next_step = NextStepLeft;
                goto loop;
            }
@@ -810,30 +813,29 @@ loop:
            break;
 
        case NextStepUp:
-           for (;;)
+           came_from = current;
+           current = current->parent;
+           if (current == NULL)
+           {
+               iter->is_over = true;
+               break;      /* end of iteration */
+           }
+           else if (came_from == current->right)
+           {
+               /* return current, then continue to go up */
+               break;
+           }
+           else
            {
-               came_from = iter->last_visited;
-               iter->last_visited = iter->last_visited->parent;
-               if (iter->last_visited == NULL)
-               {
-                   iter->is_over = true;
-                   break;      /* end of iteration */
-               }
-
-               if (came_from == iter->last_visited->right)
-               {
-                   /* return current, then continue to go up */
-                   break;
-               }
-
                /* otherwise we came from the left */
+               Assert(came_from == current->left);
                iter->next_step = NextStepRight;
                goto loop;
            }
-           break;
    }
 
-   return iter->last_visited;
+   iter->last_visited = current;
+   return current;
 }
 
 /*