CODEWITHSUNDEEP
c++
Apple_Banana
Apple_Banana

Reputation: 131

Bug when creating a class without using the new keyword

So I have a node class. I add a single node to a std::vector if mouse is pressed. This is the way I did it the first time.

struct Node
{
    Node(int _x, int _y) : x(_x), y(_y)
    {
    };
    int x = 0; 
    int y = 0;
    Node* neighbours = nullptr;
};

std::vector<Node*> nodes;
//The following is in a while loop
if (GetMouse(0).bPressed) {
    Node node = Node(mouseX - (nodeSize / 2), mouseY - (nodeSize / 2));
    nodes.push_back(&node);
}

What I was hoping is that a node would be created and x, y position of that node would be set to mouse x and mouse y. Then the address of that node would be added to the vector.

That isn't quite what happens. After struggling for a bit, while printing all the x and y positions of nodes in the list, I noticed that they were all the same (position of the latest added node). I am struggling to see why this happens.

After searching google for a bit, I replaced the code with this and it works fine.

if (GetMouse(0).bPressed){
    Node* node = new Node(mouseX - (nodeSize / 2), mouseY - (nodeSize /2));
    nodes.push_back(node);
}

It works now and i just want to know why all the positions were same the first time.

Upvotes: 0

Views: 40

Answers (1)

Remy Lebeau
Remy Lebeau

Reputation: 597156

Your original code is pushing a pointer to a local variable that goes out of scope immediately after the push, destroying the node and leaving the vector with a dangling pointer, eg:

if (GetMouse(0).bPressed)
{
    Node node = Node(mouseX - (nodeSize / 2), mouseY - (nodeSize / 2));
    nodes.push_back(&node);
} // <-- node is destroyed here!

Using new instead mitigates that issue, by allocating the node dynamically so it is not destroyed until you destroy it explicitly. Just make sure you actually delete the nodes when you are done using them, eg:

std::vector<Node*> nodes;

...

if (GetMouse(0).bPressed)
{
    Node *node = new Node(mouseX - (nodeSize / 2), mouseY - (nodeSize / 2));
    nodes.push_back(node);
}

...

for(Node *node : nodes) {
    delete node;
}
nodes.clear();

Alternatively, use a smart pointer like std::unique_ptr and let it manage the destructions for you, eg:

std::vector<std::unique_ptr<Node>> nodes;

...

if (GetMouse(0).bPressed)
{
    auto node = std::make_unique<Node>(mouseX - (nodeSize / 2), mouseY - (nodeSize / 2));
    nodes.push_back(std::move(node));
}

...

nodes.clear();

Otherwise, change the vector to not hold pointers at all, eg:

std::vector<Node> nodes; // <-- no *

...

if (GetMouse(0).bPressed)
{
    Node node(mouseX - (nodeSize / 2), mouseY - (nodeSize / 2));
    nodes.push_back(node); // <-- no &
}

...

Though, I suspect that last one will not suit your needs in the long run due to the Node::neighbours member. So, stick with dynamic allocation, that is the typical use-case for a linked-list anyway.

Upvotes: 1

Related Questions