[libcxx-commits] [PATCH] D127418: [libc++] Use bounded iterators in std::span when the debug mode is enabled

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

LGTM after the CI passes. I leave the final approval to @philnik since he also had some comments.



================
Comment at: libcxx/include/__iterator/bounded_iter.h:85
+    // Classes allowed to use this iterator:
+    template <class, size_t> friend class _LIBCPP_TEMPLATE_VIS span;
+
----------------
ldionne wrote:
> Mordante wrote:
> > How about including `<span>` ?
> > 
> > I don't feel adding all options here is the cleanest approach. But I expect this list to remain very short so I don't object.
> We can't include `<span>` because it includes this header! That being said, I changed this to a free function that creates these `__bounded_iter`s instead, since it allows me to test them.
Thanks! I even wonder whether the previous dependencies would work for modules.


================
Comment at: libcxx/include/__iterator/bounded_iter.h:200
+        return __x.__current_ == __y.__current_;
+    }
+
----------------
ldionne wrote:
> Mordante wrote:
> > Can we make all these operators only available before C++20 and add `operator<=>` for C++20 and later.
> I'm really not sure it's worth the added complexity. Am I missing a benefit this would provide, like efficiency or something else? If not, given that we *have* to define the operators to support older C++ Standards anyway, it seems to me that supporting `<=>` would only add additional complexity.
Mainly for consistency with other containers and iterators. I don't know whether there are places in the Standard that would mandate this. (I haven't spend much time reading the spaceship wording.) But I don't see it as a blocker.


================
Comment at: libcxx/test/libcxx/iterators/bounded_iter/types.compile.pass.cpp:39
+static_assert(std::is_same<BoundedIter::iterator_concept, Iterator::iterator_concept>::value, "");
+#endif
----------------
I really like this set of `__bounded_iter` tests!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127418