Vector elements in c++ always set to last element

Question

I want to store members of a class into a vector. In my class, I have some private variables that I access via constant pointers so that they are protected from being changed (see this post). Problem: When I add instances of the class into a vector during a loop and access them afterwards, all elements seem to be the last one added. Here's a MWE:

#include <iostream>
#include <vector>

using namespace std;

class myClass {
private:
    int x_;

    void init(int x) {
        x_ = x;
    }

public:
    const int &x;

    myClass(int x) : x(x_) {
        init(x);
    };
};

int main(int argc, const char * argv[]) {
    vector<myClass> myVector;

    // Does not work
    for (int j=0; j<3; j++) {
        myVector.push_back(myClass(j));
    }
    for (int j=0; j<3; j++) {
        cout << "1st attempt: " << myVector.at(j).x << endl;
    }
    myVector.clear();

    // Works
    for (int j=0; j<3; j++) {
        myVector.push_back(myClass(j));
        cout << "2nd attempt: " << myVector.at(j).x << endl;
    }
    myVector.clear();

    // Works also
    myVector.push_back(myClass(0));
    myVector.push_back(myClass(1));
    myVector.push_back(myClass(2));
    for (int j=0; j<3; j++) {
        cout << "3rd attempt: " << myVector.at(j).x << endl;
    }

    return 0;
}

Ovious question: What am I doing wrong and can I fix it?

Answer 1

What am I doing wrong and can I fix it?

Well this idea looks misguided in general. Putting that aside: if you want to "fix it" you need to think about what is happening when you push_back:

for (int j=0; j<3; j++) {
    myVector.push_back(myClass(j));
}

You are passing the class by value, so the copy constructor is invoked. Hence push_back isn't getting myClass(j), it's getting a copy. Yet you didn't write a copy constructor...so where is your copy constructor coming from? You're getting the compiler default, and it's just going to say the copy's int &x gets the same value as the original (as opposed to the new object's x_.)

This means that the reference inside of the copy in the vector is to the original object you passed in as myClass(j). Yet that original parameter is disposed after the push_back call is finished. The memory space is probably reused each time through the loop which is why you see the last stale value. Try running this through Valgrind or similar, it will probably pick up a problem.

You can get around this by writing your own copy constructor that does what you actually meant:

class myClass {
private:
    int x_;

    void init(int x) {
        x_ = x;
    }

public:
    const int &x;

    myClass(int x) : x(x_) {
        init(x);
    };

    myClass(myClass const & other) : x_ (other.x_), x(x_) {
    }
};

This way you are explicitly telling the compiler about your implicit desire to link the int const & to the int. If you don't tell it, it has no way of knowing they are related at all.

But there are probably more than a few reasons not to do this kind of interface in the first place. Just use a normal accessor method unless you've thought of a really good reason to do otherwise.

(Take note also of @JoachimPileborg's suggestion of emplace_back, which is good to know about.)

Answer 2

To expand on my comment...

When you do

myVector.push_back(myClass(0));

you create a temporary object with myClass(0), an object which is destructed once the push_back function have returned. This temporary object is copied into the vector, and I think that the the member variable x in the copy will reference the member variable x_ of the temporary object. That will of course lead to undefined behavior when you later use x which now references data in a destructed object.

There are two ways of solving this:

1. Use emplace_back instead of push_back.
2. Add a copy-constructor which makes x reference the objects own x_ variable.

Answer 3

When you add an object to a vector it is copied across, invoking the class' copy constructor. Because myClass has a reference member, the default copy constructor won't work for the semantics you want, so you need to define your own:

myClass (const myClass& rhs) : x(x_) {
    init(rhs.x);
}

While you're at it, you might want to define an assignment operator as well:

myClass& operator= (const myClass& rhs) {
    x_ = rhs.x;
    return *this;
}

Answer 4

it's easily fixed by repairing the definition of myClass, thus:

class myClass {
private:

public:
    const int x;

    myClass(int x_) : x(x_) {
    };
};

rational:

myClass implements the data-is-interface idiom which is fine, but if you're going to expose data do so by exposing the actual data - not a reference to it.

Remember that a reference is really a pointer underneath so it contains a memory address, and also that its presence in your class prevents the automatic generation of a move constructor.