push_back keeps rewriting all entries in the vector to the item pushed_back?

Question

I am currently trying to make a light copy of a vector. A light copy meaning that the content of the copy, is a vector of pointer of the original vector. I am for some reasons having problems doing so... Each time I push_back a new entry into the copy vector, I seem to overwrite all the entries in the vector, to the element I just push_back, why is this happening:

MWE:

// This file is a "Hello, world!" in C++ language by GCC for wandbox.
#include <iostream>
#include <cstdlib>
#include <experimental/filesystem>
#include <vector>
#include <algorithm>


struct abstract_data_structure
{
    int x;
    int y;
    std::vector<int> points;
    std::string name;

    abstract_data_structure(int x, int y , std::vector<int> points, std::string name)
    {
        this->x= x;
        this->y = y;
        this->points = points;
        this->name = name;
    }
};

int main()
{
     std::vector<abstract_data_structure> storage;
     std::vector<abstract_data_structure*> copy_of_storage;

     std::srand(std::time(0));
     for (int i = 0; i< 5 ; i++)
     {     
        int x = std::rand();
        int y = std::rand();
        std::vector<int> vecOfRandomNums(10);
        std::generate(vecOfRandomNums.begin(), vecOfRandomNums.end(), []() {
           return rand() % 100;
        });
        std::string name = "something"+std::to_string(i);
        abstract_data_structure something(x,y,vecOfRandomNums,name);
        storage.push_back(something);

     }

    std::cout << storage.size() << std::endl;
    std::cout << "Make the copy" << std::endl;

    for (auto element : storage)
    {
        std::cout << "in storage" << std::endl;
        std::cout << element.name << std::endl;
        copy_of_storage.push_back(&(element));
        std::cout << "in copy storage" << std::endl; 

        for (auto copy_element: copy_of_storage)
        {
            std::cout << copy_element->name << std::endl; 
        }
    }


}

https://wandbox.org/permlink/Xn96xbIshd6RXTRa

which outputs this:

5
Make the copy
in storage
something0
in copy storage
something0
in storage
something1
in copy storage
something1
something1
in storage
something2
in copy storage
something2
something2
something2
in storage
something3
in copy storage
something3
something3
something3
something3
in storage
something4
in copy storage
something4
something4
something4
something4
something4
0

As one might see are all the entries in the copy vector, modified to point to last element inserted? why?

Answer 1

//***1***
for (auto element : storage)
{
    std::cout << "in storage" << std::endl;
    std::cout << element.name << std::endl;
    //***2***
    copy_of_storage.push_back(&(element));
    std::cout << "in copy storage" << std::endl; 

    for (auto copy_element: copy_of_storage)
    {
        std::cout << copy_element->name << std::endl; 
    }
}

At ***1***, you're making a local copy of each variable.

At ***2***, you're taking the address of that local copy and saving it in the vector.

When you leave scope, the object has been destructed and no longer exists. So when you later try to read it, you get Undefined Behavior, which means you cannot guarantee the behavior that will take place.

What seems to be happening is that each time, you're taking the address of the local variable, and each iteration of the for-loop, that location of the variable is the same, so each address in the final copy vector is the same, and because the most recent copy is whichever entry in the storage vector you were most recently working with, that's the version that gets read by the pointer.

If you change ***1*** to for(auto & element : storage), your issue should be fixed.

Answer 2

The problem is a bit deeper than you may suspect. In

std::vector<abstract_data_structure> storage;
std::vector<abstract_data_structure*> copy_of_storage;

copy_of_storage cannot be a copy of storage without a lot of extra effort and probably dynamic allocation of abstract_data_structures.

copy_of_storage contains pointers and those pointers have to point to abstract_data_structures. These abstract_data_structures may or may not be copies.

The naive fix, swapping

for (auto element : storage)

for

for (auto & element : storage)

makes element a reference to an abstract_data_structure stored within storage. This means that element references a variable that has longer life than a single iteration of the for loop, but

copy_of_storage.push_back(&(element));

doesn't make a copy. It just points to the original abstract_data_structure inside storage. Sometimes this is useful, but when one list is supposed to be a copy of the other, it probably should directly refer to the other list.

For example, adding another abstract_data_structure to storage will invalidate the pointers. The contents of storage can be moved in the existing storage. The storage may be replaced with a larger block and the pointers left pointing to invalid memory. All sorts of nasty things can happen.

Instead, I recommend

std::vector<abstract_data_structure> storage;
std::vector<abstract_data_structure> copy_of_storage;

and later

for (auto & element : storage)

and

copy_of_storage.push_back(element);

Now you have a copy.

But... there are easier ways. std::vector is a very smart beast. You can

copy_of_storage = storage;

and storage will copy itself.

Recommended extra reading: What is The Rule of Three? This is not required for your example, but very very important to learn early in a programming career.