double free or corruption (fasttop) when using erase in vector. How can you erase several items of a vector knowing their indexes?

Question

I am working on a "big" project and I am stuck with a segmentation fault which I finally narrowed to a simpler and reproducible example:

I am going to put two pieces of code. First:

#include <iostream>
#include <string>
#include <vector>
using namespace std;
int main()
{

    std::vector<int> someVector;

    printf("1\n");
    someVector.push_back(1);

    printf("2\n");
    someVector.push_back(2);

    printf("3\n");
    someVector.push_back(3);


    someVector.erase(someVector.begin());
    someVector.erase(someVector.begin());

    return 0;
}

Here, everything works fine. Ok. Now let's try something a little more complicated. Instead of using a vector of ints, lets use a vector of my own defined class.

Note: I'm no expert and I may have made errors when defining the classes. The reason why there is a copy constructor in the class definition is because the parameters variable, is actually a pointer to another class (and not an int), which I copy on the body of the constructor on the real example.

#include <iostream>
#include <string>
#include <vector>
using namespace std;

class ActionType {
  public:
    std::string name;
    int type;
    int * parameters;

    ActionType(const std::string _name, const int _type
        );

    ActionType(const ActionType &);   

    ~ActionType(){
        // Direct implementation of destructor
        printf("DELETING\n");
        delete parameters;
    }

  };

 // Implementation of initializer      
 ActionType::ActionType(const std::string _name, const int _type)
        : name(_name)
        , type(_type)
        , parameters(new int(5))
{
}

 // Implementation of copy constructor
 ActionType::ActionType(const ActionType & actionType)
     : name(actionType.name)
      , type(actionType.type)
      , parameters(new int(*actionType.parameters))

{
    printf("COPYING\n");
}

int main()
{
    ActionType actionType("foo", 1);

    std::vector<ActionType> actions;

    printf("1\n");
    actions.push_back(actionType);

    printf("2\n");
    actions.push_back(actionType);

    printf("3\n");
    actions.push_back(actionType);

    actions.erase(actions.begin());
    actions.erase(actions.begin());

    return 0;
}

This, which should be pretty much the same, throws the error:

*** Error in `./a.out': double free or corruption (fasttop): 0x0000000001618c70 ***                             
Aborted (core dumped)

The thing is, in my real example I need a way to delete several items of the vector, and to do that I have something like:

for (int i (...)){
    int indexToDelete = getIndex();
    vector.erase(vector.begin()+indexToDelete);
}

I could also use something like:

std::vector<int> indexes;
for (int i (...)){
    int indexToDelete = getIndex();
    indexes.push_back(indexToDelete);
}
vector.erase(indexes);

But I haven't figured any function that does this. I've seen other answers where they use sort to put all items to remove at the end and after they call pop_back, but it didn't seem super clean.

Anyway, in summary:

• Does somebody know why when using a class with vector the function erase doesn't work as good?

• How can I delete several items knowing their indexes in a vector?

Answer 1

You are missing the assignment operator for your class. The std::vector requires that your class has correct copy semantics, and your ActionType does not have correct copy semantics.

It is simple to add an assignment operator for the class you've written:

#include <algorithm>
//...
class ActionType
{
   //...
   ActionType& operator=(const ActionType &);
   //...
};

ActionType& ActionType::operator=(const ActionType &rhs)
{
    if ( this != &rhs )
    {
        ActionType temp(rhs);
        std::swap(temp.name, name);
        std::swap(temp.type, type);
        std::swap(temp.parameters, parameters);
    }  // <-- The temp is destroyed here, along with the contents.
    return *this;
}

The above uses the copy / swap idiom to implement the assignment operator.

Note that the code now works when the assignment operator is added.

Edit:

The alternate form of the assignment operator could be:

class ActionType
{
   //...
   ActionType& operator=(ActionType);
   //...
};

ActionType& ActionType::operator=(ActionType temp)
{
    std::swap(temp.name, name);
    std::swap(temp.type, type);
    std::swap(temp.parameters, parameters);
    return *this;
}

Answer 2

When you delete the parameters array you need to supply a []

delete [] parameters;

Otherwise you get undefined behavior in your program.

Sidenote > It would probably better to keep it as a std::vector or std::array instead.