Calling push_back on a std::vector of objects (not pointers) has serious side effects. So would pointers be better?

Question

This follows on from my previous question, where I was slow to supply all the information so I am creating a new question in the hope of more input. Given:

    struct otherClass {
        ImportantObj    *ptrToImpObj;
        otherClass() {ptrToImpObj = NULL;}
    };
    struct anEntry {
        Thing *thing;
        std::vector<otherClass> *iDM2P2H;
        anEntry(Thing *tg, std::vector<sqdDMPair> *dM2Pair = NULL)
            : thing(tg), iDM2P2H(dM2Pair) {}
        ~anEntry() {delete iDM2P2H;}
    };
    std::vector<anEntry *> aQueue;
    std::vector<anEntry> bQueue;
    void aMethod () {
        Thing thingy = &(masterArrayOfThings[42]);
        aQueue.push_back(new anEntry(thingy, iDM2P2H));
    }
    void bMethod () {
        Thing thingy = &(masterArrayOfThings[42]);
        bQueue.push_back(anEntry(thingy, iDM2P2H));
    }

The second method will invoke the dtor on the memory shared between the two objects involved with the copy constructor.

In this case I feel I should be using pointers inside my vector and aQueue is preferable to bQueue.

Thanks.

_EDIT_

Let's say I'll have 20 aQueue's and they will be cleared and iDM2P2H replaced hundreds (thousands?) of times a second as the AI route evaluation sees fit.

Answer 1

About deletion of iDM2P2H, either now or ever in the future, your program is going to cause that error. If you set the same pointer in two objects, sooner or later they will both die and their destructors will try to delete the same memory. If you use pointers and new the objects, the problem persists when you delete the anEntry objects.

The solution is simply to avoid deleting iDM2P2H in the anEntry destructor, and delete it in the same context of whoever created it. That is for example if it was created at program startup, you could delete it when you have finished your need for it, in the main execution path of the program.

Answer 2

Your problem here is your anEntry copy constructor is broken. The default copy constructor (anEntry (const anEntry &)) simply copies all members; with your class's explicit destructor, this results in double freeing. The same applies for the default operator=. Per the Rule of Three, if you define any one of a destructor, copy constructor, and operator=, you should generally implement the other two (or forbid them by making them private and unimplemented); otherwise there's a good chance the default implementation of one of them will cause problems like this.

Now, the vector class requires a working copy constructor. This is part of the vector contract. If your class has a missing (ie, forbidden by making it private) copy constructor, the compiler will error out, preventing these "serious side effects". If the class has a broken copy constructor, well, that's not vector's fault.

Note that you may want to consider using RAII-style allocation in your anEntry class. For example, make iDM2P2H a std::vector<otherClass> instead of std::vector<otherClass> *. By doing so, you won't need a destructor at all, and if the default copy semantics are acceptable to you, you might be able to do with the default copy constructor in this case.

That said, vector's copying may indeed entail significant overhead at times. There are a number of things you can do to work around this; however I would recommend against a raw std::vector<anEntry *> - the reason being that this won't clean up the pointed-to elements automatically.

Instead, use a std::vector<std::unique_ptr<anEntry>> (if you have a C++0x compiler) or boost::ptr_vector<anEntry>. This will give you the automatic destruction benefits of vector but will not copy any elements (as the vector is a vector of pointers to objects). Note that in the unique_ptr case you will need to use std::move to add elements to the vector.

Alternately, if your compiler supports C++0x move-aware containers, you could write a move constructor:

struct anEntry {
    Thing *thing;
    std::vector<sqdDMPair> iDM2P2H;

    anEntry(Thing *thing_, std::vector<sqdDMPair> *vec = NULL)
      : thing(thing_), iDM2P2H(vec ? *vec : std::vector<sqdDMPair>())
    { }

    // Default copy constructor and destructor OK

    // Move constructor
    anEntry(anEntry &&src)
      : thing(src.thing), iDM2P2H(std::move(src.iDM2P2H)) { }

    anEntry &operator=(anEntry &&other) {
      if (this != &other) {
        thing = other.thing;
        iDM2P2H = std::move(other.iDM2P2H);
      }
    }
};

Using std::move allows the contents of iDM2P2H to be moved to the new position in the outer vector without copying recursively. However, C++0x support is still in its infancy, and your compiler and STL may or may not support it yet (if std::vector<std::unique_ptr<int>> compiles, you're probably ok).

Answer 3

It depends on how expensive and big your objects are. Having a vector of objects is ok if they're small, but if they're big or expensive¹, all the copying/constructing/destructing can add up to a large performance hit, in which case you should use pointers (and then you have to manage the memory for all the objects, etc).

¹ A class can be small and expensive if it doesn't have many data members (so it's not big in size) but manages expensive resources that are changed when it is copied, destroyed, or created.

Answer 4

If you really have objects that contain pointers to shared data/resources, maybe you could consider using std::shared_ptr<my_shared_type> rather than just having delete called in the dtor.

Using std::vector<> of pointers might make the issues go away for awhile, but it's potentially just masking a more fundamental issue of shared resource management.

Hope this helps.