[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 inline comments.


================
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()));
+    }
+  }();
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117368