Fixup tests that believe them to be so. Most notably including some heavy refactoring in std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp, which now detects pointers and validates that iterator_concept is present only for pointers.
Details
- Reviewers
• Quuxplusone ldionne - Group Reviewers
Restricted Project - Commits
- rG2d653b7e5b35: [libcxx][test] array and basic_string_view iterators are not portably pointers
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp | ||
---|---|---|
24–25 | This initially struck me as the wrong fix — this test is supposed to be checking the iterator-sentinel ctor, not a pointer-sentinel ctor! But I see now that the old code was never trying to test any non-pointer iterator types anyway. Which is... also bad. :P template<class CharT, class It, class Sent> constexpr void test() { auto val = MAKE_STRING_VIEW(CharT, "test"); It first = It(val.data()); Sent last = Sent(It(val.data() + val.size()); auto sv = std::basic_string_view<CharT>(first, last); assert(sv.data() == val.data()); assert(sv.size() == 4); } and then we should test with, like, test<char, char*, char*>(); test<char, char*, const char*>(); test<char, const char*, sentinel<const char*>>(); // is this supposed to work? test<char, const char*, sized_sentinel<const char*>>(); test<char, contiguous_iterator<char*>, contiguous_iterator<char*>>(); test<char, contiguous_iterator<char*>, sized_sentinel<contiguous_iterator<char*>>>(); // Test other character types #ifndef TEST_HAS_NO_WIDE_CHARACTERS test<wchar_t, wchar_t*>(); #endif test<char8_t, char8_t*>(); test<char16_t, char16_t*>(); test<char32_t, char32_t*>(); | |
libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/end.pass.cpp | ||
27 | These added to_addresses seem uncontroversial to me. (@Mordante, these make sense to you too?) Maybe you want to pull them out into a separate PR and/or commit them immediately. | |
libcxx/test/support/test_iterators.h | ||
359 ↗ | (On Diff #400193) | What necessitated this? I'd prefer to fix the users, instead of legalizing what they're doing. |
libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp | ||
---|---|---|
24–25 | I've also just realized that the ASSERT_SAME_TYPE which intends to test the deduction guide is doing nothing since we don't use CTAD on this line. Cleanup time. | |
libcxx/test/support/test_iterators.h | ||
359 ↗ | (On Diff #400193) | Code wrapping array and/or string_view iterators in contiguous_iterator - I don't recall precisely where. If the intent is to only admit pointers, why are we using iterator_traits<It>? |
libcxx/test/std/strings/string.view/string.view.deduct/iterator_sentinel.pass.cpp | ||
---|---|---|
23–26 | Surely string_view::iterator is never char* (because it would be const char*). But anyway this if is redundant because if iterator actually were a pointer, then val.begin() and val.data() would be the same value of the same type and thus completely interchangeable. Just use .data() (as you did above). | |
libcxx/test/support/test_iterators.h | ||
359 ↗ | (On Diff #400193) | AIUI, the original intent of e.g. forward_iterator (which I'm going to use as the type specimen for all test iterators) was to permit wrapping weirder iterator types; but if you actually want to do that correctly, the metaprogramming gets awful. And we were never actually relying on it to work with any non-pointer type. (Except, apparently, by accident. ;)) In D116613 I suggested that we ought to permit forward_iterator<stride_counter<int*>>, to cut down on the absolutely abominable metaprogramming involved with our current stride_counting_iterator<forward_iterator<int*>>. If we did, then that would be a reason to remove this assertion... but we still might want to keep some kind of assertion, because I would bet money that e.g. forward_iterator<std::vector<bool>::iterator> would explode. |
LGTM except for Arthur's comments, which I agree with (but IMO removing the static_assert in contiguous_iterator is fine).
I checked everything and I originally thought the libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp changes were weakening the tests for libc++, but it turns out they don't.
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
---|---|---|
52–53 | Here and in the functions below too. We generally don't use a single template parameter named O. |
Moar review comments
libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp | ||
---|---|---|
24–25 |
Wrong test - the deduction guide test is just below. ASSERT_SAME_TYPE here is copy-pasta. | |
libcxx/test/std/strings/string.view/string.view.deduct/iterator_sentinel.pass.cpp | ||
23–26 | This is ridiculous. Fixing and refactoring. | |
libcxx/test/support/test_iterators.h | ||
359 ↗ | (On Diff #400193) | I'll revert this change. Oddly, I'm finding no test fallout now - I assume the problems have either been corrected elsewhere since I first found this, or are fixed in later commits on my branch. |
LGTM; none of my remaining nits are blocking.
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp | ||
---|---|---|
102–103 | FWIW, I'm predictably un-thrilled by the overloading of test<It, V, C> and test<It, V, D, R, P, C>; I would rather they were named differently, like testOrdinary and testUnusual, or test and test5, or something; but I have no really good naming suggestion, and this is still an improvement over the status quo, so no action required unless you feel like it. | |
libcxx/test/std/strings/string.view/string.view.deduct/iterator_sentinel.pass.cpp | ||
24 | Consider s/test_deduction/test_ctad/g for clarity. | |
libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/advance_to.pass.cpp | ||
42 | Both on lines 29,32 and 42,45, consider spelling the right-hand side of the == as just fmt + 1. @CaseyCarter, for my own curiosity, would MSVC be happy with assert(context.begin() == view.begin() + 1); or is that considered problematic because of comparing iterators into two different string_views (even though they view the same underlying range)? ...Hmm, looks like we're doing exactly that on line 33 of format.parse.ctx/end.pass.cpp. So maybe it's OK to do it here too? |
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp | ||
---|---|---|
102–103 | I see now that test<I, V, C>, testConst<I, V, C>, and testIOterator are equivalent to calling test with different template arguments. I'll rename the first to testMutable, and refactor all of them to actually call test to eliminate duplication. | |
libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/advance_to.pass.cpp | ||
42 |
For basic_string_view we interpret "same underlying sequence" to mean "both iterators were obtained from basic_string_views with the same first and last character", so that would be a problem. (And yes, it's a problem in end. I have more patches to submit, I've been trying to keep a relatively constant five outstanding reviews.) |
One more whitespace/grammar nit in a comment. Ship it!
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp | ||
---|---|---|
121 |
FWIW, I'm predictably un-thrilled by the overloading of test<It, V, C> and test<It, V, D, R, P, C>; I would rather they were named differently, like testOrdinary and testUnusual, or test and test5, or something; but I have no really good naming suggestion, and this is still an improvement over the status quo, so no action required unless you feel like it.