[libcxx-commits] [PATCH] D118400: [libc++] Remove operator-> from the iterator archetypes

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

Quuxplusone added a subscriber: CaseyCarter.
Quuxplusone added a comment.

I don't agree with this direction, but I think the only hill I'll particularly die on is "don't reopen namespace std."



================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/arrow.pass.cpp:46-56
+    {
+      Common common(iter);
+      std::same_as<IteratorWithArrow> auto result = common.operator->();
+      assert(base(result) == buffer);
+    }
+
+    {
----------------
Consider
```
{
  Common common(iter);
  std::same_as<IteratorWithArrow> auto r1 = static_cast<Common&>(common).operator->();
  std::same_as<IteratorWithArrow> auto r2 = static_cast<Common&&>(common).operator->();
  std::same_as<IteratorWithArrow> auto r3 = static_cast<const Common&>(common).operator->();
  std::same_as<IteratorWithArrow> auto r4 = static_cast<const Common&&>(common).operator->();
  assert(base(r1) == buffer);
}
```
just to hit the move cases as well. (If `IteratorWithArrow` were move-only, would there be no `operator->`? ....waitaminute. You have an infinite loop in your `operator->`, don't you? `IteratorWithArrow::operator->()` can't //return another// `IteratorWithArrow`! It should be returning `int*`.)

(And I think I just remembered that `common_iterator` doesn't work with move-only iterator types, right? because it's meant as an adaptor to produce a C++17 iterator, which must be copyable by definition?)


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp:94
     static_assert(std::same_as<IterTraits::difference_type, std::ptrdiff_t>);
-    static_assert(std::same_as<IterTraits::pointer, const Iter&>);
+    static_assert(std::same_as<IterTraits::pointer, int*>);
     static_assert(std::same_as<IterTraits::reference, int&>);
----------------
The code on the left strikes me as weird. But whatever, I'm getting deja vu to a conversation with @CaseyCarter where he said nobody cares about the value of `::pointer`. :)


================
Comment at: libcxx/test/support/test_iterators.h:253-264
+namespace std {
+    template <class It>
+    struct pointer_traits<::contiguous_iterator<It>> {
+        using pointer = ::contiguous_iterator<It>;
+        using element_type = typename pointer_traits<It>::element_type;
+        using difference_type = typename pointer_traits<It>::difference_type;
+        template <class Other>
----------------
ldionne wrote:
> Quuxplusone wrote:
> > (A) please don't reopen namespace std, but also (B) please don't specialize `pointer_traits`.
> > Is your goal just to make `contiguous_iterator` a contiguous iterator while (for some reason) removing its `operator->`? You can do that by giving `contiguous_iterator` a member `using element_type = typename pointer_traits<It>::element_type;`.
> > https://stackoverflow.com/questions/65712091/in-c20-how-do-i-write-a-contiguous-iterator
> > 
> > (A) please don't reopen namespace std, but also (B) please don't specialize pointer_traits.
> 
> Can you please explain why that's bad? That seems gratuitous -- `pointer_traits` is explicitly made to be customized, no? Arguably, if I could remove all that complexity and only have `element_type`, that would indeed better, but I don't see why specializing `pointer_traits` would be bad in itself. **Edit: Nope, that doesn't work, I have to keep `operator->()`.**
> 
> > Is your goal just to make `contiguous_iterator` a contiguous iterator while (for some reason) removing its `operator->`?
> 
> Yes, that's my goal. The reason is to make `congituous_iterator` more minimal, just like the other changes we did to e.g. `cpp17_input_iterator` to remove default-constructibility. Because it makes it more useful for testing.
> 
> (A) please don't reopen namespace std
https://quuxplusone.github.io/blog/2021/10/27/dont-reopen-namespace-std/

> (B) please don't specialize pointer_traits
Because user iterators won't specialize pointer_traits. I suppose there //is// a tension here between "Keep test code simple; eliminate metaprogramming; make test code realistic" and "Make test code that stress-tests the corner cases." For example, we `=delete` the comma operator on all of these iterators (and should delete `operator&` too, probably) — that's more complicated than necessary, and isn't realistic, but it //does// stress-test the corner cases. So maybe "let's specialize pointer_traits for all these iterators to make sure all our algorithms respect the specialized pointer_traits" is a good suggestion on par with "Let's =delete the comma operator for all these iterators."  //Personally// I don't think it meets the bar; it's a //lot// of complexity for what it gives us IMO, especially compared to the simplicity of
https://stackoverflow.com/questions/65712091/in-c20-how-do-i-write-a-contiguous-iterator


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118400