[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 added a comment.

Thanks for working on this, I like the general direction!

I think we should add more tests to validate whether the bound iterator's operations work as expected. Also in language modes other than C++20.



================
Comment at: libcxx/include/__iterator/bounded_iter.h:66
+    _LIBCPP_HIDE_FROM_ABI __bounded_iter& operator=(__bounded_iter const&) = default;
+
+private:
----------------
Why not adding move construction and assignment?


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


================
Comment at: libcxx/include/__iterator/bounded_iter.h:143
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
+    friend __bounded_iter operator+(__bounded_iter const& __self, difference_type __n) _NOEXCEPT {
+        __bounded_iter __tmp(__self);
----------------
Same for the other places.


================
Comment at: libcxx/include/__iterator/bounded_iter.h:191
+    friend bool operator>(__bounded_iter const& __x, __bounded_iter const& __y) _NOEXCEPT {
+        return __x.__current_ == __y.__current_;
+    }
----------------
Copy paste `==` and in the next operators.


================
Comment at: libcxx/include/__iterator/bounded_iter.h:200
+        return __x.__current_ == __y.__current_;
+    }
+
----------------
Can we make all these operators only available before C++20 and add `operator<=>` for C++20 and later.


================
Comment at: libcxx/include/__iterator/bounded_iter.h:210
+    _Iterator __current_; // current iterator
+    _Iterator __begin_, __end_; // valid range represented as [begin, end)
+};
----------------



================
Comment at: libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp:13
+// REQUIRES: has-unix-headers
+// UNSUPPORTED: !libcpp-has-debug-mode, c++03
+
----------------
c++03 is already unsupported at line 8.


================
Comment at: libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp:26
+        Foo array[] = {{0}, {1}, {2}};
+        std::span<Foo> const span(array, 3);
+        {
----------------



================
Comment at: libcxx/test/libcxx/containers/views/views.span/span.elem/assert.back.pass.cpp:17
+// REQUIRES: has-unix-headers
+// UNSUPPORTED: c++03
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{10.9|10.10|10.11|10.12|10.13|10.14|10.15|11.0|12.0}}
----------------
Please have a look at the other tests too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127418