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.
Paths
| Differential D139235
Reapply "[libc++][ranges]Refactor `copy{,_backward}` and `move{,_backward}`" ClosedPublic Authored by var-const on Dec 2 2022, 3:26 PM.
Details
Summary 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
Unit TestsFailed Event Timelineldionne added inline comments.
philnik added inline comments.
var-const marked 4 inline comments as done. Comment ActionsAddress feedback, further expand testing.
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.
This revision is now accepted and ready to land.Dec 8 2022, 12:09 PM Comment Actions The title of the commit could be changed to "Reapply" instead of "Revert Revert" -- that's always a bit confusing IMO.
var-const retitled this revision from Revert "Revert "[libc++][ranges]Refactor `copy{,_backward}` and `move{,_backward}`"" to Reapply "[libc++][ranges]Refactor `copy{,_backward}` and `move{,_backward}`".Dec 17 2022, 1:55 PM var-const added inline comments.
This revision was landed with ongoing or failed builds.Jan 13 2023, 4:57 PM Closed by commit rG5629d492df38: Reapply "[libc++][ranges]Refactor `copy{,_backward}` and `move{,_backward}`" (authored by varconst <varconsteq@gmail.com>, committed by var-const). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions Heads-up that we're seeing crashes due to the recommit of this change. Comment Actions
Thank you for the heads-up. Can you provide more details? 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. Comment Actions @alexfh @asbirlea @rupprecht Thank you for calling this out. Added notes about potential breakages of existing code to the release notes in https://github.com/llvm/llvm-project/commit/abda3e523ade7377b03a25dc2b6192dbe855c567
Revision Contents
Diff 489117 libcxx/include/CMakeLists.txt
libcxx/include/__algorithm/copy.h
libcxx/include/__algorithm/copy_backward.h
libcxx/include/__algorithm/copy_move_common.h
libcxx/include/__algorithm/move.h
libcxx/include/__algorithm/move_backward.h
libcxx/include/__algorithm/ranges_copy.h
libcxx/include/__algorithm/ranges_copy_backward.h
libcxx/include/__algorithm/ranges_copy_n.h
libcxx/include/__algorithm/ranges_move.h
libcxx/include/__algorithm/ranges_move_backward.h
libcxx/include/__algorithm/ranges_set_difference.h
libcxx/include/__algorithm/ranges_set_symmetric_difference.h
libcxx/include/__algorithm/ranges_set_union.h
libcxx/include/__algorithm/rotate.h
libcxx/include/__algorithm/set_difference.h
libcxx/include/__algorithm/set_symmetric_difference.h
libcxx/include/__algorithm/set_union.h
libcxx/include/__iterator/reverse_iterator.h
libcxx/include/__type_traits/is_always_bitcastable.h
libcxx/include/algorithm
libcxx/include/module.modulemap.in
libcxx/include/valarray
libcxx/test/libcxx/algorithms/alg.modifying.operations/copy.pass.cpp
libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_nontrivial.pass.cpp
libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp
libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_unwrap_reverse.pass.cpp
libcxx/test/libcxx/private_headers.verify.cpp
libcxx/test/libcxx/transitive_includes/cxx2b.csv
libcxx/test/libcxx/type_traits/is_always_bitcastable.compile.pass.cpp
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp
libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp
libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp
libcxx/test/std/time/time.cal/time.cal.day/time.cal.day.nonmembers/ostream.pass.cpp
libcxx/test/std/time/time.duration/time.duration.nonmember/ostream.pass.cpp
libcxx/test/std/time/time.syn/formatter.duration.pass.cpp
libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
libcxx/test/std/utilities/format/format.functions/P2418.pass.cpp
libcxx/test/std/utilities/format/format.tuple/format.functions.vformat.pass.cpp
|
I would make this a proper boolean trait instead. It may make it easier to reuse elsewhere outside of SFINAE contexts.