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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 4 inline comments as done.
ldionne added a comment.
Herald added a project: All.

In D118400#3287820 <https://reviews.llvm.org/D118400#3287820>, @Quuxplusone wrote:

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

Hopefully we can agree that this is an improvement if I keep `operator->` as a way to define `contiguous_iterator`?



================
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);
+    }
+
+    {
----------------
Quuxplusone wrote:
> 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?)
Fixed by removing `IteratorWithArrow` altogether.

> And I think I just remembered that common_iterator doesn't work with move-only iterator types, right?

Yes, that's right.


================
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>
----------------
Quuxplusone wrote:
> 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
Okay, I sat on this for a while, but I think I agree. In the next iteration I'll remove `operator->` only from the archetypes that truly don't need it for anything, like `cpp17_input_iterator` & friends. I'll keep it on `contiguous_iterator`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118400