Should std::vector::erase() destroy the element being erased? (instead of the last element)

Question

Background

A while back, I ran into some behaviour that I found very strange and seemingly incorrect and I filed a bug report with GCC about it. You can see the report and the response I got here:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47305

(I'm going to replicate most of that here.)

At the time, I didn't understand the answer, but was not a member of StackOverflow and didn't have anyone to ask about it, so I just hacked a work-around and went on. But recently, I was revisiting this code and I still don't understand the rationale for this not being a bug, so...

My Question

In the C++ stdlib distribution included with with my Mac (currently OS X, Darwin 12.2.0 x86_64), the implementation of std::vector::erase() from /usr/include/c++/4.2.1/vector.tcc lines 106-116 is shown here:

template<typename _Tp, typename _Alloc>
  typename vector<_Tp, _Alloc>::iterator
  vector<_Tp, _Alloc>::
  erase(iterator __position)
  {
    if (__position + 1 != end())
      std::copy(__position + 1, end(), __position);
    --this->_M_impl._M_finish;
    this->_M_impl.destroy(this->_M_impl._M_finish);
    return __position;
  }

Note that destroy() will be called for the element that is last in the vector prior to the call to this erase(), instead of being called for the element pointed to by __position. I believe this is incorrect -- I think it should instead call destroy() for the element pointed to by __position. For simple POD types, this isn't that big of a deal, but for classes where the destructors have side effects (such as smart pointers), it can be critical.

The following code illustrates the problem:

#include <vector>
#include <iostream>

class MyClass
{
    int m_x;
public:
     MyClass(int x) : m_x(x) { }
    ~MyClass()
    {
        std::cerr << "Destroying with m_x=" << m_x << std::endl;
    }
};

int main(void)
{
    std::vector<MyClass> testvect;
    testvect.reserve(8);
    testvect.push_back(MyClass(1));
    testvect.push_back(MyClass(2));
    testvect.push_back(MyClass(3));
    testvect.push_back(MyClass(4));
    testvect.push_back(MyClass(5));

    std::cerr << "ABOUT TO DELETE #3:" << std::endl;

    testvect.erase(testvect.begin() + 2);

    std::cerr << "DONE WITH DELETE." << std::endl;

    return 0;
}

When I compile this with g++ version 4.2.1 (no command line arguments) on my Mac, it produces the following when I run it:

Destroying with m_x=1
Destroying with m_x=2
Destroying with m_x=3
Destroying with m_x=4
Destroying with m_x=5
ABOUT TO DELETE #3:
Destroying with m_x=5
DONE WITH DELETE.
Destroying with m_x=1
Destroying with m_x=2
Destroying with m_x=4
Destroying with m_x=5

Note that the key line after the "ABOUT TO DELETE #3" message shows that the destructor was actually called for the (copy of the) fifth thing I added. Importantly, the destructor for #3 is never called!!

It appears that the version of erase() that takes a range (two iterators) also has a similar problem.

So my question is, am I wrong to expect that the destructor of the element I am erasing from a vector gets called? It seems that if you can't count on this, you can't safely use smart pointers in vectors. Or is this just a bug in the STL vector implementation distributed by Apple? Am I missing something obvious?

Answer 1

It's a reasonable point, there are at least two different ways that you could think to implement erase:

• destroy element 3, then copy-construct element 3 from 4, then 4 from 5, then destroy 5.
• copy-assign to 3 from 4, then to 4 from 5, then destroy 5.

C++11 introduces a third way to do it:

• move-assign to 3 from 4, then to 4 from 5, then destroy 5.

In fact for vector::erase the first way is forbidden by the C++03 standard in 23.2.4.3/4:

Complexity: The destructor of T is called the number of times equal to the number of the elements erased, but the assignment operator of T is called the number of times equal to the number of elements in the vector after the erased elements.

Although this text is designed primarily to indicate the runtime complexity of the operation, you see that it mandates the second implementation. C++11 says the same thing with "move assignment" in place of "assignment".

There's also a more fundamental problem with the first way, which is that in general (although not for int and hence not for MyClass either), copying can fail. If erase destroyed the third element of the vector, and then the copy from the 4th element failed then the vector would be in a rather dangerous state -- the third element isn't a proper object any more. So the restriction in the standard does rather more than just define the runtime, it prevents this bad failure case.

Answer 2

The destructor only get called for the last element, but the object being erased gets overwritten by assigning from the next element to it. So the assignment operator frees up the old resources. When the type is a smart pointer, that means doing adjusting the reference and, if appropriate, deleting the controlled object.

Answer 3

Actually, there is no problem. In the line

std::copy(__position + 1, end(), __position);

the deleted element gets overwritten with consecutive elements; if it holds resources that need to be freed, it would do so in its operator=.

In C++11, you would want to use move instead of copy; but what you posted is an OK C++03 implementation for std::vector::erase.

Answer 4

When you erase the element containing a 3, the following elements have to be shifted back to fill the void. Then element #3 gets assigned what #4 has, and #4 gets assigned what #5 has. The last element, #5, is left with whatever value it has since it is about to be deleted anyway.

When the vector goes out of scope, you see the remaining 4 elements being destroyed.

If you were to hold smart pointers in your vector, the resources will be properly freed when the assignment operator is called.