[libc++] Fix copy-paste damage in ranges::rotate_copy and its test

[StephanTLavavej]

Found while running libc++'s tests with MSVC's STL.

ranges::rotate_copy takes forward_iterators as this test's comment banner correctly depicts. However, this test had bogus assertions expecting that ranges::rotate_copy would be constrained away for not-quite-bidi iterators. @philnik777 confirmed that these were copy-paste relics from the ranges::reverse_copy test.

I fixed this by replacing the assertions with the test types that aren't quite forward iterators/ranges. Additionally, I noticed that the top-level test() function was missing coverage with the weakest possible forward_iterator<int*>.

This revealed that the product code in ranges_rotate_copy.h was similarly damaged. In addition to fixing it by taking forward_iterator and forward_range as depicted in the Standard, this drops the inclusion of <__iterator/reverse_iterator.h> as this algorithm doesn't need std::__reverse_range.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

Found while running libc++'s tests with MSVC's STL.

ranges::rotate_copy takes forward_iterators as this test's comment banner correctly depicts. However, this test had bogus assertions expecting that ranges::rotate_copy would be constrained away for not-quite-bidi iterators. @philnik777 confirmed that these were copy-paste relics from the ranges::reverse_copy test (which does need bidi).

I fixed this by replacing the assertions with the test types that aren't quite forward iterators/ranges. Additionally, I noticed that the top-level test() function was missing coverage with the weakest possible forward_iterator<int*>.

Note: The fact that this test was passing for you is quite curious. From a quick glance, I don't see a problem with the libc++ product code constraints, so I'm not sure where the behavior difference is.

---

Full diff: https://github.com/llvm/llvm-project/pull/74544.diff

1 Files Affected:

• (modified) libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/ranges.rotate_copy.pass.cpp (+5-4)

diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/ranges.rotate_copy.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/ranges.rotate_copy.pass.cpp
index 1f18d787c2f4c..58b0f75d9b5f2 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/ranges.rotate_copy.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/ranges.rotate_copy.pass.cpp
@@ -34,16 +34,16 @@ template <class Range, class Out = int*>
 concept HasRotateCopyR = requires(Range range, Out out) { std::ranges::rotate_copy(range, nullptr, out); };
 
 static_assert(HasRotateCopyIt<int*>);
-static_assert(!HasRotateCopyIt<BidirectionalIteratorNotDerivedFrom>);
-static_assert(!HasRotateCopyIt<BidirectionalIteratorNotDecrementable>);
+static_assert(!HasRotateCopyIt<ForwardIteratorNotDerivedFrom>);
+static_assert(!HasRotateCopyIt<ForwardIteratorNotIncrementable>);
 static_assert(!HasRotateCopyIt<int*, SentinelForNotSemiregular>);
 static_assert(!HasRotateCopyIt<int*, SentinelForNotWeaklyEqualityComparableWith>);
 static_assert(!HasRotateCopyIt<int*, OutputIteratorNotIndirectlyWritable>);
 static_assert(!HasRotateCopyIt<int*, OutputIteratorNotInputOrOutputIterator>);
 
 static_assert(HasRotateCopyR<UncheckedRange<int*>>);
-static_assert(!HasRotateCopyR<BidirectionalRangeNotDerivedFrom>);
-static_assert(!HasRotateCopyR<BidirectionalRangeNotDecrementable>);
+static_assert(!HasRotateCopyR<ForwardRangeNotDerivedFrom>);
+static_assert(!HasRotateCopyR<ForwardRangeNotIncrementable>);
 static_assert(!HasRotateCopyR<UncheckedRange<int*, SentinelForNotSemiregular>>);
 static_assert(!HasRotateCopyR<UncheckedRange<int*>, OutputIteratorNotIndirectlyWritable>);
 static_assert(!HasRotateCopyR<UncheckedRange<int*>, OutputIteratorNotInputOrOutputIterator>);
@@ -112,6 +112,7 @@ constexpr void test_out_iterators() {
 }
 
 constexpr bool test() {
+  test_out_iterators<forward_iterator<int*>>();
   test_out_iterators<bidirectional_iterator<int*>>();
   test_out_iterators<random_access_iterator<int*>>();
   test_out_iterators<contiguous_iterator<int*>>();

[CaseyCarter]

Note: The fact that this test was passing for you is quite curious. From a quick glance, I don't see a problem with the libc++ product code constraints, so I'm not sure where the behavior difference is.

Try restoring the original static_asserts but negating their expected results. The failure diagnostics will probably be illuminating.

[StephanTLavavej]

Good idea - which I didn't even need to try, as extending the test coverage with forward_iterator<int*> revealed that the product code was also copy-pasted saying bidirectional:

llvm-project/libcxx/include/__algorithm/ranges_rotate_copy.h

Lines 35 to 56 in 06c5c27

	namespace __rotate_copy {
	struct __fn {
	template <bidirectional_iterator _InIter, sentinel_for<_InIter> _Sent, weakly_incrementable _OutIter>
	requires indirectly_copyable<_InIter, _OutIter>
	_LIBCPP_HIDE_FROM_ABI constexpr rotate_copy_result<_InIter, _OutIter>
	operator()(_InIter __first, _InIter __middle, _Sent __last, _OutIter __result) const {
	auto __res1 = ranges::copy(__middle, __last, std::move(__result));
	auto __res2 = ranges::copy(__first, __middle, std::move(__res1.out));
	return {std::move(__res1.in), std::move(__res2.out)};
	}
	template <bidirectional_range _Range, weakly_incrementable _OutIter>
	requires indirectly_copyable<iterator_t<_Range>, _OutIter>
	_LIBCPP_HIDE_FROM_ABI constexpr rotate_copy_result<borrowed_iterator_t<_Range>, _OutIter>
	operator()(_Range&& __range, iterator_t<_Range> __middle, _OutIter __result) const {
	return (*this)(ranges::begin(__range), std::move(__middle), ranges::end(__range), std::move(__result));
	}
	};
	} // namespace __rotate_copy
	inline namespace __cpo {
	inline constexpr auto rotate_copy = __rotate_copy::__fn{};

Let's see if I can fix this despite not being tooled up to work on libc++ product code.

[philnik777]

Thanks for the fix! LGTM with green CI.

[StephanTLavavej]

Thanks, CI is green with only one spurious build failure on Apple. Merging now.