Return class object pointer from another class

Question

I'm having difficulties explaining the problem with words. But I have replicated my problem in a minimal example. Basically I have a main class holding a vector of a different class objects. This main class has a member function that creates an object, add it to the vector, and returns a pointer to the new object in the vector. When I then try to access the values of the object from the pointer that was returned, the values are as if they were never set.

Here is the code of a minimal example:

EDIT:

As some pointed out I had value = value in the member function. This was obviously a mistake, and I've edited to _value = value

#include <iostream>
#include <vector>

class MySecondaryClass
{
public:
    MySecondaryClass(int aValue) { _aValue = aValue; }
    ~MySecondaryClass() {}
    int _aValue;
};

class MyMainClass
{
private:
    std::vector<MySecondaryClass> _secClassVec;
public:
    MyMainClass() {}
    ~MyMainClass(){}

    MySecondaryClass* addNewSecClass(int aValue) { 
        MySecondaryClass newSecClass(aValue);
        _secClassVec.push_back(newSecClass);
        return &_secClassVec[_secClassVec.size() - 1];
    }
};


int main() {

    MyMainClass mainObject;
    MySecondaryClass* secObject1_ptr = mainObject.addNewSecClass(1);
    MySecondaryClass* secObject2_ptr = mainObject.addNewSecClass(2);
    MySecondaryClass* secObject3_ptr = mainObject.addNewSecClass(3);

    std::cout << secObject1_ptr->_aValue << std::endl;
    std::cout << secObject2_ptr->_aValue << std::endl;
    std::cout << secObject3_ptr->_aValue << std::endl;

    return 0;
}

And the output:

-572662307
-572662307
3

Expected output is:

1
2
3

Can someone explain what I'm doing wrong here?

Answer 1

Name your constructor argument something that doesn't hide your member in the constructor body, and start using initialzier lists:

MySecondaryClass(int aValue_) : aValue(aValue_)
{ }

Also remember that pushing back into a vector may invalidate iterators (and pointers to its elements).

---

Well, now that you edited you question. I can only suggest you return a proxy object which behaves like a pointer.

class MySecondaryClassPtr
{
  MyMainClass& mmc_;
  std::size_t  pos_;

  public:
  MySecondaryClassPtr(MyMainClass& mmc, std::size_t pos) :
   msc_(msc), pos_(pos)
  {}

  MySecondaryClass& operator*()  { return mmc_._secClassVec[pos_]; }
  MySecondaryClass* operator->() { return &mmc_._secClassVec[pos_]; }  

};

This "smart pointer" proxy object is tied to the lifetime of your MyMainClass object, rather than the internal storage of the vector.

Answer 2

First problem is this in the constructor:

MySecondaryClass(int aValue) { aValue = aValue; }

what it does is that is assigns the parameter aValue to itself, i.e. does not set the aValue attribute of the class. This can be fixed by either:

MySecondaryClass(int aValue) { this->aValue = aValue; }

or

MySecondaryClass(int aValue): aValue(aValue) {}

But in general, this is why it is not recommended to name the parameters the same as the attributes.

Second problem is, that the call of:

MySecondaryClass* secObject3_ptr = mainObject.addNewSecClass(3);

might invalidate the memory assigned to secObject1_ptr and secObject2_ptr (if the vector resizes) - basically every call to mainObject.addNewSecClass() might invalidate the previously returned pointers (technically it is undefined behavior).

EDIT

To answer the comment question: If you want to store the actual objects in the class and access them, you need to do it differently, either by index, or in some cases you can keep a map by id, depending on what you need. But you normally cannot keep the pointers to the changing data structure.

One possibility:

class MyMainClass
{
private:
    std::vector<MySecondaryClass> _secClassVec;
public:
    MyMainClass() {}
    ~MyMainClass() {}

    size_t addNewSecClass(int aValue) {
        MySecondaryClass newSecClass(aValue);
        _secClassVec.push_back(newSecClass);
        return _secClassVec.size() - 1;
    }

    const MySecondaryClass& operator [](size_t idx) const
    {
        return _secClassVec[idx];
    }
};


int main() {

    MyMainClass mainObject;

    size_t secObject1_idx = mainObject.addNewSecClass(1);
    size_t secObject2_idx = mainObject.addNewSecClass(2);
    size_t secObject3_idx = mainObject.addNewSecClass(3);

    std::cout << mainObject[secObject1_idx].aValue << std::endl;
    std::cout << mainObject[secObject2_idx].aValue << std::endl;
    std::cout << mainObject[secObject3_idx].aValue << std::endl;

    return 0;
}

However also this will only work, if you only add to the vector. Removal will invalidate the indices after the removed item as well.

Another possibility might be to allocate dynamically and only keep the pointers in the vector - then the original object address will not change when adding items. But then comes the necessity to manage the lifetime, copy construction etc., which can be somewhat mitigated by using a smart pointer (e.g. shared_ptr).

Answer 3

When you push into a vector it has to allocate memory for the new element.

This allocation demand might not be satisfiable at the very location in memory where the underlaying array is currently at, so the vector has to allocated contiguous memory for its whole content elsewhere, move the old content, add the new one and free the previously allocated block.

So pointers into the vector's memory can become invalid every time you change the vector.