[libcxx-commits] [PATCH] D118400: [libc++] Remove operator-> from the iterator archetypes
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Mar 10 09:02:35 PST 2022
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
More information about the libcxx-commits
mailing list