small_vector move is broken

[mrmiller <michael_miller@…>]

Moving a small_vector from a to b doesn't properly clear the contents from a. This is true for both the move constructor and operator=. See the brief example below:

    boost::container::small_vector<std::unique_ptr<int>, 8> a;
    a.emplace_back(std::unique_ptr<int>{new int{1}});
    assert(a.size() == 1);
    
    auto b = std::move(a);
    assert(b.size() == 1);
    assert(a.size() == 0); // this fails

[mrmiller <michael_miller@…>]

I located a couple different problem spots that seem to fix the issue in the cases I've tested. On my end, I refactored steal_resources and broke out the resetting into clear_resources:

   void steal_resources(vector_alloc_holder &x) BOOST_NOEXCEPT_OR_NOTHROW
   {
      this->m_start     = x.m_start;
      this->m_size      = x.m_size;
      this->m_capacity  = x.m_capacity;
      x.clear_resources();
   }
   
   void clear_resources() BOOST_NOEXCEPT_OR_NOTHROW
   {
      this->m_start = pointer();
      this->m_size = this->m_capacity = 0;
   }

Every time we use a move_iterator to do the copy because the allocator isn't propagable, we need to clear out the previous allocator. That means simply adding x.clear_resources() after the call to assign in vector::priv_move_assign and small_vector::move_construct_impl.

Lastly, small_vector's move constructor seems rather suspect to me:

   small_vector(BOOST_RV_REF(small_vector) other)
      : base_type(initial_capacity_t(), internal_capacity(), ::boost::move(other.get_stored_allocator()))
   {  this->move_construct_impl(other, other.get_stored_allocator());   }

That call to other.get_stored_allocator() looks really suspicious after we've moved the allocator in the initializer...

[Ion Gaztañaga]

This is not a bug but is intended to work just like std::vector::operator= when:

• allocator_type is not propagable (e.g. when propagate_on_container_move_assignment::value is false)
• allocators from *this and other are not equal.

In that case, elements are moved one-by-one but there is no requirement to clear the source. Quoting ( ​http://en.cppreference.com/w/cpp/container/vector/operator%3D):

Move assignment operator. Replaces the contents with those of other using move semantics (...). other is in a valid but unspecified state afterwards. (...)If std::allocator_traits<allocator_type>::propagate_on_container_move_assignment() (...) is false and the source and the target allocators do not compare equal, the target cannot take ownership of the source memory and must move-assign each element individually, allocating additional memory using its own allocator as needed.

Several std implementations (I think libcpp and dinkumware) don't clear() contents of the moved container after move assignment, if allocators are not propagable and they don't compare equal. libstdc++ seems to clear the source. LLVM's SmallVector also clears the source.

std::vector does not support unequal allocators in the move constructor (moving the allocator is required to yield to equal allocators), but small_vector must (as it holds internal storage which is never propagable). So the move constructor behaves more or less like a default-constructor + move assignment.

In your example, "a" uses internal storage to hold the unique_ptr, when it's moved to "b", "b" checks if "a"'s memory can be transferred, but since it's internal, it can't be done. So it move constructs elements one by one to "b"'s internal storage (so unique_ptr's in "b" should be nulled. "b" is left in a valid but unspecified state, which is the requirement of standard containers.

It's a bit surprising, but no one should care about the state of "b", it can be safely reused (and if it has capacity, some performance benefit can be obtained), but not guaranteed to be empty. All Boost.Container containers works like this, when allocators can't be propagated so it's a design decision.