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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D127418#3570877 <https://reviews.llvm.org/D127418#3570877>, @philnik wrote:

>> This approach has the benefit that we don't need to modify the container (`std::span`) at all, unlike with the usual `__wrap_iter` and `__debug_insert_c()` methods. It doesn't use a global locking mechanism and the internal debug "database". As a result, `std::span` can stay trivially copyable and trivially destructible, which could be important for the correctness of user programs.
>
> I wouldn't call `std::span` a container.

Yeah, I'm being sloppy in my choice of terms here, indeed.

In D127418#3578865 <https://reviews.llvm.org/D127418#3578865>, @Mordante wrote:

> 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.

Agreed, and done!



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


================
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);
----------------
Mordante wrote:
> Same for the other places.
I don't think we've decided on an official east-const vs west-const policy, so I'll leave as-is. It's consistent within the file and tests.


================
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_;
+    }
----------------
Mordante wrote:
> Copy paste `==` and in the next operators.
Ooooh, thanks. I added tests, too.


================
Comment at: libcxx/include/__iterator/bounded_iter.h:200
+        return __x.__current_ == __y.__current_;
+    }
+
----------------
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.


================
Comment at: libcxx/include/__iterator/bounded_iter.h:210
+    _Iterator __current_; // current iterator
+    _Iterator __begin_, __end_; // valid range represented as [begin, end)
+};
----------------
Mordante wrote:
> 
Will leave as-is. I intended to put them on the same line since the comment then applies to both `begin` and `end`, which is what I wanted.


================
Comment at: libcxx/include/span:214
+#ifdef _LIBCPP_ENABLE_DEBUG_MODE
+    using iterator               = __bounded_iter<pointer>;
 #else
----------------
philnik wrote:
> What do you think about introducing a `constexpr bool _InDebugMode` and make this `using iterator = _If<_InDebugMode, __bounded_iter<pointer>, __wrap_iter<pointer>>`? With that it would also be possible to use `if constexpr (_InDebugMode)` and `void func() requires _InDebugMode`. This only works for C++11, C++17 and C++20 code respectively, but is a lot nicer to read IMO.
I am ambivalent about this. I don't think it's a bad idea, but I'm not sure there is much of an actual benefit beyond being more "modern". I'll give it a shot and report on what I think.


================
Comment at: libcxx/include/span:218
 #endif
     using reverse_iterator       = _VSTD::reverse_iterator<iterator>;
 
----------------
jkorous wrote:
> This gets `rbegin()` and `rend()` covered as well, right? Shall we add some tests to make the guarantee explicit?
Yes, it covers it. Added tests.


================
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}}
----------------
Mordante wrote:
> Please have a look at the other tests too.
Yeah, thanks, I fixed all of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127418