Add a regression test for erasing from a vector

[• Quuxplusone]

Summary

After rebasing my trivially-relocatable branch, this behavior was broken... but no libc++ unit test caught it! Add a regression test specifically for erasing out of a vector.

I'll do some more investigation of the actual root cause of the trivially-relocatable bug that this is a regression test for, since I have to fix it anyway. Still, I was shocked that libc++'s test suite didn't catch a simple misbehavior. (The specific misbehavior in my case was that after erasing l1.begin() you'd get {2,2,3,4} instead of {2,3,4,5} — it's shifting down only one element instead of all of them. But I haven't yet looked at my code to see why that is.)

EDIT: I have now looked at my trivially-relocatable bug. The problem was a missing * sizeof(_Tp) in the third argument to memmove here:

     pointer __p = this->__begin_ + __ps;
-    this->__destruct_at_end(_VSTD::move(__p + 1, this->__end_, __p));
+    _Tp *__rawp = _VSTD::__to_address(__p);
+    if (__vector_relocate_via_memcpy<_Tp, _Allocator>::value) {
+        __alloc_traits::destroy(this->__alloc(), __rawp);
+        --this->__end_;
+        if (__p != this->__end_) {
+            _VSTD::memmove(__rawp, __rawp + 1, this->__end_ - __p);
+        }
+    } else {
+        this->__destruct_at_end(_VSTD::move(__p + 1, this->__end_, __p));
+    }
     this->__invalidate_iterators_past(__p-1);

I still claim that it's useful to have a regression test for this bug, even though the bug itself was only local to my checkout. :)

[miscco]

Could we remove from the middle to ensure that we do not tamper with the initial elements

[ldionne]

LGTM, would be even better to add a second test like Michael suggests.

[miscco]

I mean I do not really want to bother you, but the "correct" thing to do would be to actually not use int but one of the defined arche types (in this case one of those that count how many times they have been moved from.)

This would ensure that we indeed do not move from those elements before it and only move from those elements after it.

[• Quuxplusone]

I'll add a second test along Michael's lines, if Louis doesn't beat me to it.
However, "using an arche type that counts the number of times it was moved from" would actually defeat the purpose of this regression test, as my regression happened only when erasing from a vector of trivially relocatable types that don't count the number of times they're moved from! So I saw the bug with vector<int> and vector<string>, but wouldn't see it with complicated, user-defined-move-constructor types.

[miscco]

@Quuxplusone I am totally fine with you adding the basic test.

That said, it is a "general" test that does not depend whether a variable is relocatable.

The long term solution would be to define an archetype that is relocatable (Can we actually do that, I havent looked at the paper to be honest) and test with that. However, that would indeed be a quite significant amount of work.

[ldionne]

Requesting changes so it shows up in my review queue until the additional test is added. Thanks for the diligence here @Quuxplusone .

[• Quuxplusone]

Updated with a second test for erasing from the middle of a vector.

[miscco]

Thanks a lot for improving the test coverage!

libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp

struct Throws {
     };
 
 bool Throws::sThrows = false;
 #endif
 
 int main(int, char**)
 {
     {
+    int a1[] = {1, 2, 3, 4, 5};
+    std::vector<int> l1(a1, a1+5);
+    l1.erase(l1.begin());
+    assert(is_contiguous_container_asan_correct(l1));
+    assert(l1 == std::vector<int>(a1+1, a1+5));
+    }
+    {
+    int a1[] = {1, 2, 3, 4, 5};
+    int e1[] = {1, 3, 4, 5};
+    std::vector<int> l1(a1, a1+5);
+    l1.erase(l1.begin() + 1);
+    assert(is_contiguous_container_asan_correct(l1));
+    assert(l1 == std::vector<int>(e1, e1+4));
+    }
+    {
     int a1[] = {1, 2, 3};
     std::vector<int> l1(a1, a1+3);
     std::vector<int>::const_iterator i = l1.begin();
     assert(is_contiguous_container_asan_correct(l1));
     ++i;
     std::vector<int>::iterator j = l1.erase(i);
     assert(l1.size() == 2);
     assert(distance(l1.begin(), l1.end()) == 2);