Page MenuHomePhabricator

Add a regression test for erasing from a vector
ClosedPublic

Authored by Quuxplusone on Sep 28 2020, 8:07 AM.

Details

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. :)

Diff Detail

Event Timeline

Quuxplusone created this revision.Sep 28 2020, 8:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2020, 8:07 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested review of this revision.Sep 28 2020, 8:07 AM
Quuxplusone edited the summary of this revision. (Show Details)Sep 28 2020, 8:16 AM
miscco added a subscriber: miscco.Sep 28 2020, 9:04 AM

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

ldionne accepted this revision.Sep 28 2020, 9:07 AM

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

This revision is now accepted and ready to land.Sep 28 2020, 9:07 AM

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.

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.

@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 requested changes to this revision.Sep 28 2020, 1:48 PM

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

This revision now requires changes to proceed.Sep 28 2020, 1:48 PM

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

ldionne accepted this revision.Sep 29 2020, 7:27 AM
This revision is now accepted and ready to land.Sep 29 2020, 7:27 AM
miscco accepted this revision.Sep 29 2020, 7:33 AM

Thanks a lot for improving the test coverage!

This revision was landed with ongoing or failed builds.Sep 29 2020, 9:19 AM
This revision was automatically updated to reflect the committed changes.