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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added subscribers: Mordante, Quuxplusone.
Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
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>);
----------------
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*>();
```


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/end.pass.cpp:27
     std::basic_format_parse_context<CharT> context(fmt);
-    assert(context.end() == &fmt[3]);
+    assert(std::to_address(context.end()) == &fmt[3]);
     ASSERT_NOEXCEPT(context.end());
----------------
These added `to_address`es 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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117368