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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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
----------------
crtrott wrote:
> var-const wrote:
> > 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?
> Only if one would use layout_left/right on for their own data structure. For example mdarray will use reuse the layouts, and a user implemented version of that, or for example a user implemented "matrix_type" may not itself check. Generally I do expect the layouts to be used in such types, since its useful machinery beyond mdspan. The reason not to use mdspan in that case is that users may want value semantics of a matrix type, so they just store say a double[16] and a layout_left::mapping<extents<char, 4, 4> or so. 
> 
> So the philosophical question is whether we should make the machinery for implementing such types access safe in hardened mode or not. I do believe there is value to doing so and promising essentially that stuff like the standard layouts are safe in hardened mode to be used on their own. But I am not dead-set on it. 
Thank you for explaining. I'm a little concerned about imposing a penalty on the main use case (using `mdspan` with the default layout types) for the sake of, IIUC, a rather niche use case in the hardened mode. In addition, users that reuse the layout types can do their own checking if they want, so I think covering that case is a lesser priority. Also, while we can go both ways if necessary, I'd avoid removing assertions from the hardened mode if possible, so I'd rather start small and grow than vice versa. So that being said, I'd avoid double-checking in this release, with the potential to revisit this for LLVM 18. IIUC, this means that in the hardened mode:
- users of `mdspan` will get all the benefits of checking without paying a potential extra cost for the double check (however small that cost might be);
- users who implement their own data structures using `layout_left`/`layout_right` would need to do their own element access checking.

If so, I'd avoid double-checking, at least for this release.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156181