CODEWITHSUNDEEP
c++
tomashauser
tomashauser

Reputation: 571

Struct pointers are overriding each other

I have a function:

    void initGraph(node_t *node, const int orderedInputNodes[]) {
    int index;
    
    for (int i = 0; i < S * 2; i++) {
        if (orderedInputNodes[i] == node->number) {
            index = i % 2 == 0 ? i + 1 : i - 1;

            node_t newNode;
            newNode.number = orderedInputNodes[index];
            newNode.left = EMPTY;
            newNode.right = EMPTY;

            if (node->left == EMPTY) {
                node->left = &newNode;
            }
            else if (node->right == EMPTY) {
                node->right = &newNode;
            }
        }
    }
    .
    .
    .
}

Everytime I find a corresponding number I create newNode, assign its value to it and pass it either to the left or to the right, however, when the code comes to the part when the right child is being assigned, the left child gets overwritten. How come is that? I thought by stating node_t newNode; a completely new node will be automatically created.

Upvotes: 0

Views: 80

Answers (1)

David Schwartz
David Schwartz

Reputation: 182819

I thought by stating node_t newNode; a completely new node will be automatically created.

Correct. The problem is that the new node can replace the old node. Consider:

 for (int j = 0; j < 1000; ++j)
 {
     int i;
     i = 3;
 }

You don't think after this loop runs, there's a thousand instances of i still sitting around, do you? Every time this bit of code runs, a new i is created. But every time this bit of code runs, the i from the previous iteration doesn't exist anymore because it goes out of scope.

You have:

    {
        node_t newNode; // newNode is created here
        newNode.number = orderedInputNodes[index];
        newNode.left = EMPTY;
        newNode.right = EMPTY;

        if (node->left == EMPTY) {
            node->left = &newNode; // you save a pointer to it here
        }
        else if (node->right == EMPTY) {
            node->right = &newNode;
        }
    }
          // but newNode doesn't exist anymore here

So you've saved a pointer to an object that no longer exists. It winds up pointing to whatever happens to be stored in that memory, likely the next newNode that is created on the next pass in the loop.

Don't store pointers to local objects.

Update: You say you're coding in C++. Why are you using raw pointers then? Do you really want to manually manage the lifetimes of objects? C++ has fantastic tools to make this easy like std::unique_ptr.

Upvotes: 2

Related Questions