0

Consider the following function which erases a node from a binary search tree if the node has no children:

void erase_no_children(node* todel)
{
    //...
    if (todel->parent->left == todel) //if todel is left child
        todel->parent->left = nullptr;

    if (todel->parent->right == todel) //if todel is right child
        todel->parent->right = nullptr;

    delete todel;
}

Since todel->parent->left == todel that means that by setting todel->parent->left to nullptr, I'm just as well setting todel to nullptr. The compiler doesn't complain at all.

Question: Is this safe to do? Does it leak? Or is it undefined behaviour?

DeiDei
  • 10,205
  • 6
  • 55
  • 80
  • 3
    You're not setting `todel` to `nullptr`. – Barmar Dec 08 '15 at 17:26
  • No, you aren't setting todel to nullptr. You're setting todel->parent->left to nullptr. From what's shown here, this is safe and no memory leaks are present. Remember these are pointers. All they do is point at something. You can have 100 pointers that point to the same memory, and setting one to nullptr doesn't affect the other 99. – Christopher Schneider Dec 08 '15 at 17:26
  • 1
    Setting the value of `todel->parent->left` does not change the value of `todel`. – Galik Dec 08 '15 at 17:27
  • Reopening, because while the OP's question is already answered in the duplicate mark, there is a fundamental misunderstanding here that needs to be answered. – Sebastian Redl Dec 08 '15 at 17:27
  • @SebastianRedl Thanks for that. I was just about to when you did. – NathanOliver Dec 08 '15 at 17:29

3 Answers3

7

Since todel->parent->left == todel that means that by setting todel->parent->left to nullptr, I'm just as well setting todel to nullptr.

That's not correct. todel and todel->parent->left are distinct pointer variables; setting one to nullptr doesn't affect the other.

So you're not deleting nullptr (which would be safe and a no-op).

Sebastian Redl
  • 69,373
  • 8
  • 123
  • 157
2

Your function is making sure that your node can't be reached (i.e. making it an orphan) so that it is safe to delete its memory. Your assignment of the Right & Left values to nullptr is perfectly acceptable, as it's stating that the node is no longer a child of its parent.

You are freeing the memory that was allocated to the C++ program by writing delete todel. This can only occur the one time. Once you've released the memory back to the OS, any further pointers to that variable are now "Stale" (i.e: out of date)

You are doing the correct thing in managing each pointer individually (tying off loose ends) and then deleting the remaining pointer.

Setting todel->parent->left = nullptr does not affect the pointer value contained in todel. It also does not release the memory that was allocated from the OS to todel. Your delete statement does; all of your assignments to nullptr are doing are removing other references to that memory.

Toby Speight
  • 27,591
  • 48
  • 66
  • 103
J. Murray
  • 1,460
  • 11
  • 19
1

todel->parent->left and todel->parent->right are different pointers to todel. If they are pointing at the same object as todel is, then it is good practice to reset them to point to null. If you leave them set, you then have 'dangling pointers' which can cause problems that are notoriously hard to debug.

Note in particular that delete todel; means "I have finished with the object that todel points at." That might not be true if there's still some other variable pointing at that object!

Toby Speight
  • 27,591
  • 48
  • 66
  • 103