[libcxx-commits] [PATCH] D69466: Guard against overflow in span::subspan

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added a comment.

Some small comments as I read it again



================
Comment at: libcxx/include/span:305
     {
         _LIBCPP_ASSERT(__idx >= 0 && __idx < size(), "span<T,N>[] index out of bounds");
         return __data[__idx];
----------------
The first condition is trivially true due to the change to `size_t` (`__idx >= 0`)


================
Comment at: libcxx/include/span:305
     {
         _LIBCPP_ASSERT(__idx >= 0 && __idx < size(), "span<T,N>[] index out of bounds");
         return __data[__idx];
----------------
miscco wrote:
> The first condition is trivially true due to the change to `size_t` (`__idx >= 0`)
We should add a static_assert for `_Extent > 0` similar to `front()` and `back()`


================
Comment at: libcxx/include/span:317
     {
-        static_assert(_Extent > 0, "span<T,N>[].back() on empty span");
+        static_assert(_Extent > 0, "span<T,0>[].back() on empty span");
         return __data[size()-1];
----------------
I should remove "[]" same for `front()`


================
Comment at: libcxx/include/span:474
     {
         _LIBCPP_ASSERT(__idx >= 0 && __idx < size(), "span<T>[] index out of bounds");
         return __data[__idx];
----------------
Same here, `__idx` is a `size_t`so `__idx >= 0` is always true


================
Comment at: libcxx/test/std/containers/views/span.elem/elem.fail.cpp:34
+  {
+    [[maybe_unused]] auto s1 = sp.back(); // expected-error-re at span:* {{static_assert failed{{( due to requirement '.*')?}} "span<T,0>[].back() on empty span"}}
+  }
----------------
Should adopt the error message with something like `span<T,0>::back()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69466