This reverts commit a6e1080b87db8fbe0e1afadd96af5a3c0bd5e279.
Fix the conditions when the memmove optimization can be applied and refactor them out into a reusable type trait, fix and significantly expand the tests.
Differential D139235
Reapply "[libc++][ranges]Refactor `copy{,_backward}` and `move{,_backward}`" var-const on Dec 2 2022, 3:26 PM. Authored by
Details
This reverts commit a6e1080b87db8fbe0e1afadd96af5a3c0bd5e279. Fix the conditions when the memmove optimization can be applied and refactor them out into a reusable type trait, fix and significantly expand the tests.
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions I'll have another look but this LGTM w/ comments applied. Thanks a lot for all the work on this patch, the rabbit hole is much deeper than I initially thought.
Comment Actions The title of the commit could be changed to "Reapply" instead of "Revert Revert" -- that's always a bit confusing IMO.
Comment Actions Heads-up that we're seeing crashes due to the recommit of this change. Comment Actions This is going to be extremely difficult to back out since we've made changes on top of that. Please work with us to understand this issue and fix it forward ASAP. In particular, we will want to make sure it lands in LLVM 16 as early as possible. Thanks! Comment Actions Not a breakage report, but just a comment in case it wasn't expected/intended: it looks like this increases the conformance requirement for custom iterators passed to std::copy and friends. From a couple tests, it seems like gcc/libstdc++ already has stricter requirements than libc++, so the conformance requirement is being matched between libc++ and libstdc++ now. As an example: struct MyIterator { using iterator_category = std::forward_iterator_tag; using value_type = float; // These two lines are now also needed // using pointer = float*; // using reference = const float&; using difference_type = std::ptrdiff_t; value_type operator*(); MyIterator& operator++(); friend bool operator==(const MyIterator&, const MyIterator&); friend bool operator!=(const MyIterator&, const MyIterator&); }; void func() { std::vector<float> dest; std::copy(MyIterator(), MyIterator(), std::back_inserter(dest)); } https://godbolt.org/z/jTdbT4xcP It seems pretty clear the change here is correct, this is just an FYI of the impact & in case anyone else bumps into this. Comment Actions We've found the cause of the problems @asbirlea reported above: violation of aliasing rules. Something similar to this snippet: https://gcc.godbolt.org/z/3Y9GEMW4e Thus, again, the change is correct, but it's worth mentioning in the release notes that more aggressive optimizations in std::copy and friends may expose a lot more UB around std::vector and not only. |