[libcxx-commits] [PATCH] D156181: [libc++] Categorize mdspan assertions, and move assertions tests

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision.
var-const added a comment.
This revision is now accepted and ready to land.

LGTM with one question (and a green CI). Thanks!



================
Comment at: libcxx/include/__mdspan/extents.h:177
+        // e.g. using my_mdspan_t = mdspan<int, extents<int, 10>>; my_mdspan_t = m(new int[5], 5);
+        // Right-hand-side construction looks ok with allocation and size matchgin,
+        // but since (potentially elsewhere defined) my_mdspan_t has static size m now things its range is 10 not 5
----------------
Nit: `s/matchgin/matching/`.


================
Comment at: libcxx/include/__mdspan/extents.h:178
+        // Right-hand-side construction looks ok with allocation and size matchgin,
+        // but since (potentially elsewhere defined) my_mdspan_t has static size m now things its range is 10 not 5
+        _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
----------------
Nit: `s/things/thinks/` (I think).


================
Comment at: libcxx/include/__mdspan/layout_left.h:103
+    // not catching this could lead to out-of-bounds access later when used inside mdspan
+    // Note: since this is constraint to rank 1: extents itself would catch the invalid conversion first
+    //       and thus this assertion should never be triggered, but keeping it here for consistency
----------------
Nit: `s/rank 1:/rank 1,/`.


================
Comment at: libcxx/include/__mdspan/mdspan.h:191
   _LIBCPP_HIDE_FROM_ABI constexpr reference operator[](_OtherIndexTypes... __indices) const {
-    _LIBCPP_ASSERT_UNCATEGORIZED(__mdspan_detail::__is_multidimensional_index_in(extents(), __indices...),
-                                 "mdspan: operator[] out of bounds access");
+    // Note the standard layouts would also check this, but user provided ones may not, so we
+    // check the precondition here
----------------
Is there any case where if we don't do the check inside `layout_left`/`layout_right`, this assertion in `mdspan` would not catch it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156181