Vector erase function deleting wrong object

Question

I have a vector declared as:

vector<Curve> workingSet;

Curve is a class I've created that contains a string "name" and an array of structs, dynamically allocated by the constructor.

I have a loop that is supposed to delete 2 (out of 4) items from the vector.

vector<Curve>::iterator it = workingSet.begin();

    //remove tbi's children from the working set
    for (  ;  it != workingSet.end();){
        if (it->thisName == tbi.childAName ||it->thisName == tbi.childBName)
            it= workingSet.erase(it);
        else
            ++it;
        }

When the debugger reaches the .erase(it) call I can see that the 'it' iterator is pointing to curve 2 in the vector. This is good; I want curve 2 to be removed from the vector.

I'm then taken by the debugger to the destructor (I have a breakpoint there), which presumably should be destructing curve 2. But when I look at the 'this' watch, I can see that the curve being destructed is curve 4! The destructor then proceeds to 'delete[]' the array in the object, as required and sets the array pointer to NULL.

When the debugger returns to the program, having completed the erase() call, I can see that vector 2 has been removed from the array and curve 4 is still there. Curve 4's array pointer still points to the same location as before, but the memory has been deallocated and the array contains garbage.

Can anyone suggest why curve 4 is being messed with?

N.b. (1) There is a copy constructor for the curve class, which does a 'deep' copy. N.b. (2) There is more to the class/program than I've mentioned here

Btw, the array pointers in curves 2 and 4 point to different locations and contain different values, according to the debugger.

Edit: I've now implemented copy assignment. Now the correct item seems to be being erased from the vector, but the wrong destructor is still being called! However, when the debugger returns to the array curve 4 is still intact.

Answer 1

It appears to me that vector::erase cannot be used with a vector of locally constructed non-trivial data types (objects not constructed on the heap, the proper name escapes me right now). I have the exact same behavior you describe, the last element gets destructed twice (especially dangerous if your object has memory that is freed by the destructor) and the element that you removed is never destructed. I do not know why this happens, but it is a pitfall to watch out for.

Here is one way to fix the problem:

#include <iostream>
#include <vector>
#include <memory>

using namespace std;

class MyClass 
{
   public:
    int *x;
    MyClass(int x)
    {
       cout << "MyClass Constructor " << x << endl;
       this->x = new int(x);
    }
    MyClass(const MyClass& obj)
    {
       this->x = new int(*obj.x);
       cout << "copy constructor " << *this->x << endl;
    }
    ~MyClass()
    {
       cout << "MyClass Destructor " << *x << endl;
       delete x;
    }
};
int main(int argc, char* argv[])
{
   // incorrect
   vector<MyClass> bad_vect;
   for(int i=0;i<3;i++){
      bad_vect.push_back(MyClass(i));
      // causes a bunch of copying to happen.
      // std::move does not seem to fix this either
      // but in the end everything gets constructed as we'd like
   }
   cout << " ---- " << endl;
   bad_vect.erase(bad_vect.begin() + 1); // we expect this to remove item with x = 1 and destruct it.
   // but it does NOT do that, it does remove the item with x=1 from the vector
   // but it destructs the last item in the vector, with x=2, clearly
   // not what we want. The destructor for object with x=1 never gets called
   // and the destructor for the last item gets called twice.
   // The first time with x=2 and since that memory is freed, the 2nd time
   // it prints garbage. Strangely the double-free doesn't crash the prog
   // but I wouldn't count on that all the time.
   // Seems some parts of STL have pitfalls with locally constructed objects
   cout << " ------------ " << endl;
   // below fixes this
   vector<unique_ptr<MyClass> >vect;
   for(int i=0;i<3;i++){
      unique_ptr<MyClass> ptr(new MyClass(i));
      vect.push_back( move( ptr )); // move is required since unique_ptr can only have one owner
      // or the single one-liner below
      //vect.push_back( move( unique_ptr<MyClass>(new MyClass(i)) ));
   }
   // the above prints out MyClass Constructor 0,1,2, etc
   vect.erase(vect.begin() + 1); // remove the 2nd element, ie with x=1
   // the above prints out MyClass Destructor 1, which is what we want

   for(auto& v : vect){
     cout << *(v->x) << endl;
   } // prints out 0 and 2
   return 0; // since we're using smart pointers, the destructors for the
           // remaining 2 objects are called. You could use regular pointers
           // but you would have to manually delete everything. shared_ptr
           // also works and you don't need the move(), but with a bit more overhead
}

Answer 2

When an item is erased from the vector, all elements behind it are shifted towards the front to fill the empty space. If your compiler doesn't support move yet, it's done by copying all the elements, and the last item in the vector, which is now copied to the item before it, is a duplicate and is getting deleted.

At least that's how it should work.