This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][test] array and basic_string_view iterators are not portably pointers
ClosedPublic

Authored by CaseyCarter on Jan 14 2022, 4:49 PM.

Details

Summary

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.

Diff Detail

Event Timeline

CaseyCarter requested review of this revision.Jan 14 2022, 4:49 PM
CaseyCarter created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 14 2022, 4:49 PM
Quuxplusone requested changes to this revision.Jan 16 2022, 11:22 AM
Quuxplusone added inline comments.
libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp
22–23

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.

This revision now requires changes to proceed.Jan 16 2022, 11:22 AM
CaseyCarter marked an inline comment as done.Jan 16 2022, 9:48 PM
CaseyCarter added inline comments.
libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp
22–23

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>?

CaseyCarter marked an inline comment as done.

Address some review comments.

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).
This test deserves the same refactoring that you did above, e.g. it should test contiguous_iterator<const char*> and so on.

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. ;))
Mantra: "If you're not testing it, it doesn't work." So since we never (intentionally) used these types with non-pointer Its, it just makes sense to static_assert against that usage in the future. (Or, by accident, on other platforms in the present!)

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.

ldionne accepted this revision as: ldionne.Jan 17 2022, 11:24 AM
ldionne added a subscriber: ldionne.

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
54–55

Here and in the functions below too. We generally don't use a single template parameter named O.

CaseyCarter marked 4 inline comments as done.

Moar review comments

libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp
22–23

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.

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.

Resolve merge conflicts.

Quuxplusone accepted this revision.Feb 17 2022, 8:41 AM

LGTM; none of my remaining nits are blocking.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
55–73

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.
That is, both &fmt[1] and std::to_address(view.begin()) + 1 are verbose synonyms for 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?

This revision is now accepted and ready to land.Feb 17 2022, 8:41 AM

More refactoring!

CaseyCarter marked 3 inline comments as done.Feb 17 2022, 2:06 PM
CaseyCarter added inline comments.
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
55–73

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

@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?

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

Quuxplusone accepted this revision.Feb 19 2022, 6:52 AM

One more whitespace/grammar nit in a comment. Ship it!

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
90–92