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

Casey Carter via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Jan 17 14:04:04 PST 2022


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>);
----------------
CaseyCarter wrote:
> 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.
> 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.


================
Comment at: libcxx/test/std/strings/string.view/string.view.deduct/iterator_sentinel.pass.cpp:26-32
+  auto sv = [&] {
+    if constexpr (std::same_as<std::string_view::iterator, char*>) {
+      return std::basic_string_view(val.begin(), Sentinel(val.end()));
+    } else {
+      return std::basic_string_view(val.data(), Sentinel(val.data() + val.size()));
+    }
+  }();
----------------
Quuxplusone wrote:
> 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.
This is ridiculous. Fixing and refactoring.


================
Comment at: libcxx/test/support/test_iterators.h:359
 {
-    static_assert(std::is_pointer_v<It>, "Things probably break in this case");
-
----------------
Quuxplusone wrote:
> CaseyCarter wrote:
> > 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>`?
> 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 `It`s, 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117368



More information about the libcxx-commits mailing list