[libcxx-commits] [PATCH] D117368: [libcxx][test] array and basic_string_view iterators are not portably pointers

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter marked an inline comment as done.
CaseyCarter added inline comments.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_iterator_sentinel.pass.cpp:28
   auto val = MAKE_STRING_VIEW(CharT, "test");
-  auto sv = std::basic_string_view<CharT>(val.begin(), Sentinel(val.end()));
+  auto sv = std::basic_string_view<CharT>(val.data(), Sentinel(val.data() + val.size()));
   ASSERT_SAME_TYPE(decltype(sv), std::basic_string_view<CharT>);
----------------
Quuxplusone wrote:
> 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*>();
> ```
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.


================
Comment at: libcxx/test/support/test_iterators.h:359
 {
-    static_assert(std::is_pointer_v<It>, "Things probably break in this case");
-
----------------
Quuxplusone wrote:
> What necessitated this? I'd prefer to fix the users, instead of legalizing what they're doing.
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>`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117368/new/

https://reviews.llvm.org/D117368