CODEWITHSUNDEEP
c++qtstlvector
Colin Dumitru
Colin Dumitru

Reputation: 3455

Strange problem with vectors

I have a really strange problem with stl vectors in which the wrong destructor is called for the right object when I call the erase method if that makes any sense.
My code looks something like this:

 for(vector<Category>::iterator iter = this->children.begin(); iter != this->children.end(); iter++)
    {
        if((*iter).item == item)
        {
            this->children.erase(iter);
            return;
        }
        -------------------------
    }

It's just a simple function that finds the element in the vector which has some item to be searched, and removes said element from the vector. My problem is than when the erase function is called, and thus the object which the iterator is pointing at is being destroyed, the wrong destructor is being called. More specific the destructor of the last element in the vector is being called, and not of the actual object being removed. Thus the memory is being removed from the wrong object, which will still be an element in the vector, and the actual object which is removed from the vector, still has all of it's memory intact.
The costructor of the object looks like this:

Category::Category(const Category &from)
{
    this->name = from.name;
    for(vector<Category>::const_iterator iter = from.children.begin(); iter != from.children.end(); iter++)
        this->children.push_back((*iter));

    this->item = new QTreeWidgetItem;
}

And the destructor

Category::~Category()
{
    this->children.clear();
    if(this->item != NULL)
    {
        QTreeWidgetItem* parent = this->item->parent();
        if(parent != NULL) parent->removeChild(this->item);
        delete this->item;
    }
}

Upvotes: 2

Views: 272

Answers (3)

JaredC
JaredC

Reputation: 5300

When you erase your element from the vector, each element after it is copied (using the assignment operator) to the previous spot in the vector. Once this is complete, the last element in the vector is destructed. This could be why you're seeing your last element get destructed. Rule number one when using the STL is to ensure the copy semantics for your object are correct.

You should consider writing an assignment operator:

Category & operator =(const Category & other);

Although this may not be as simple as it sounds, considering objects will be copied and destructed many times in the vector.

Upvotes: 5

Cubbi
Cubbi

Reputation: 47438

This is expected behavior. On my implementation (and I am guessing, on yours) when erasing an element from a vector, elements from n+1 to the end are assigned into it and the very last element is destructed.

Use std::list if you don't want this to happen.

Demo:

#include <iostream>
#include <vector>
struct Category
{
        int item;
        Category(int n=0) : item(n) {}
        ~Category() { std::cout << "Category " << item << " destroyed\n"; }
};
int main()
{
        std::vector<Category> children(3);
        children[0] = Category(0);
        children[1] = Category(1);
        children[2] = Category(2);

        int item = 0;
        std::cout << " beginning the loop \n";
        for( std::vector<Category>::iterator iter = children.begin();
                                       iter != children.end(); ++iter)
        {
            if(iter->item == item)
            {
                    children.erase(iter); //  prints "Category 2 destroyed"!
                    break;
            }
        }
        std::cout << " loop done \n";
} // this will print "Category 1 destroyed" and "Category 2 destroyed"

And yes, explicit erase/remove_if is more readable than the loop.

Upvotes: 0

CashCow
CashCow

Reputation: 31445

You probably should use standard algorithms.

The main issue I see is that your destructor for Category asks its parent vector to remove it. That cannot be right, the destructor happens only when the vector is already removing it.

std::vector uses placement-new so will be calling your destructor directly. I do not know what the effect will be to go back to the vector at this point.

Remove the line if (parent != NULL ) parent->removeChild(this->item) from the destructor. It is not what you want.

Upvotes: 0

Related Questions