C++ Vector Push_back producing Weird Side Effect

Question

While I was using STL vector to store class objects,, I observed a very weird side effect, where push_back method modifies the Existing Data!

Basically I have a class containing several fields as follows:

class MyClass {
    MyClass(std::string s, int i) {
        StringValue = s;
        IntValue = i;
    }

    std::string StringValue;
    int IntValue;
}

I have a vector that contains the POINTERS to MyClass objects.. and then I am basically pushing back references to objects:

std::vector<MyClass*> MyVector;
MyClass c1("CLASS 1", 1);
MyClass c2("CLASS 2", 2);

MyVector.push_back(&c1);
// Result:
// *MyVector[0]    ==>    ("Class 1", 1)



MyVector.push_back(&c2);
// Result:
// *MyVector[0]    ==>    ("Class 2", 2)   ??why 2??
// *MyVector[1]    ==>    ("Class 2", 2)

Do you see the strange result that I got?? I've set breakpoints after each push_back statement,,, and this weird thing happened.

The first push_back statement worked fine. But then the second push_back statement MODIFIED the content of the first element, which doesn't make sense to me..

I'm assuming that it has something to do with me storing References instead of actual objects inside the vector.... but I can't figure out what is wrong.

How can I handle this issue? Any insights?

Thanks

UPDATE:

(simplified code)

MyClass c1("CLASS 1", 1);
MyClass c2("CLASS 2", 2);

MyClass temp;
while (int i=0; i<2; i++) {
    temp = c1;
    MyVector.push_back(temp);
}

You guys are right,, I get what I'm doing wrong here.. The actual object gets destructed in every loop.. What's the best way to fix this while keeping the current structure?? I'ts hard to explain but I would like to keep this structure (keeping temporary buffer outside the loop).. is this possible?

Check the actual addresses given by the pointers in the vector. Did you somehow get the same address in there twice, or did the first object somehow get turned into a copy of the second? — dlf, Jul 17 '14 at 20:59
The objects the pointers point to are probably dying before the pointers do. Post some minimal and complete code that reproduces the problem. — juanchopanza, Jul 17 '14 at 21:00
Allocate the objects using 'new', and let us know if the problem persists. — kbirk, Jul 17 '14 at 21:01
Is you copy constructor copying the pointers or making a new copy of the target object? — Thomas Matthews, Jul 17 '14 at 21:02
The code you posted works perfectly - here it is in ideone: http://ideone.com/IZxovK Your problem lies elsewhere. — RichieHindle, Jul 17 '14 at 21:02
I can't reproduce the error. Maybe you modified your objects somewhere in between — Ben, Jul 17 '14 at 21:04
If c1 and c2 had the same address, it pretty strongly suggests that c1 was destroyed before c2 was constructed and the compiler reused the late c1's address for c2 (although this isn't the case in the simplified code as shown). — dlf, Jul 17 '14 at 21:05
@user3794186: Unless you have a good reason, you should just use a `std::vector` and not have it be a vector of pointers. — Bill Lynch, Jul 17 '14 at 21:32
Stop posting code that doesn't reproduce the problem. What is the point of that? — juanchopanza, Jul 17 '14 at 21:42

RichieHindle · Accepted Answer · 2014-07-17T21:25:06.413

I'm going to out on a limb and use my psychic powers to derive the real code, rather than the simplified code in the question. Your real code looks like... drumroll...

class MyClass {
    // ...
};

void addInstance(std::vector<MyClass*>& MyVector, int i) {
    MyClass c("", i);
    MyVector.push_back(&c);
}

int main() {
    addInstance(MyVector, 1);
    addInstance(MyVector, 2);
    // ...
 }

Here's a working example to demonstrate the problem, which does indeed output "2, 2" (although that's not guaranteed because you're invoking undefined behaviour):

http://ideone.com/PxiUx9

Edit: My psychic powers said "auto variable in a function" when (now that we have the updated question) it was really "auto variable in a loop". Not too far wrong. :-)

You're storing the address of an automatic variable beyond its lifetime, which is not allowed. The address is being re-used on each call, hence each pointer stored is the same, and happens to point to the memory that was last used to store the most recently created instance of MyClass (though that's not guaranteed).

You need to (preferably) store copies of those automatic variables rather than pointers to them, or (less preferably) create them with new and later delete them with delete.

To store copies, you need to use a std::vector<MyClass>. Here's an example of how you might do that: http://ideone.com/4rIijM Note that once your class gets more complex you might need to define a copy constructor, destructor and assignment operator - look up the "rule of three". If you're using C++11, also look up the "rule of five".

Thanks! but doesnt your working example produce the wrong output as well..? shouldn't it output 1,2 instead of 2,2? — user3794186, Jul 17 '14 at 21:17
@user3794186: The first example demonstrates the problem, not a solution. Answer updated. — RichieHindle, Jul 17 '14 at 21:21
impressive, good job for finding it without even the source code! — Marine Galantin, Feb 12 '22 at 21:59

Marine Galantin · Answer 2 · 2022-02-13T00:29:19.540

For anyone looking for a clear and concise explanation:

Why does an object get destroyed when pushing it to a vector?

You are creating a temporary object:

m_foos.push_back(Foo(a));
//               ^^^^^^

Solution: The function emplace_back is what you want to use:

m_foos.emplace_back(a);

Check out: push_back vs emplace_back

EDIT--- Also, I noted that when compiling with CLion, the error is not fixed. It is also known to be an error on Visual Studio. In other words, be careful with your IDE.

C++ Vector Push_back producing Weird Side Effect

2 Answers2