[libcxx-commits] [PATCH] D151267: mdspan: implement layout_right

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this! I didn't compare the code with the Standard. A few minor nits.



================
Comment at: libcxx/docs/Status/Cxx23.rst:43
 
+   .. [#note-P0009R18] P0009R18: ``extents``, ``dextents``, ``layout_right``` are implemented.
    .. [#note-P0533R9] P0533R9: ``isfinite``, ``isinf``, ``isnan`` and ``isnormal`` are implemented.
----------------



================
Comment at: libcxx/include/__mdspan/extents.h:458
+template <integral _IndexType, class _From>
+  requires(is_integral_v<_From>)
+_LIBCPP_HIDE_FROM_ABI constexpr bool __is_multidimensional_index_in(_IndexType __extent, _From __value) {
----------------
I prefer to use concepts where possible, in `requires` and `if constexpr`. But I really would like them in requires. I still have hopes compilers can create better diagnostics for concepts than type traits. Please look at other places too.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:71
+
+  static_assert((extents_type::rank_dynamic() > 0) || __required_span_size_is_representable(extents_type()),
+                "layout_right::mapping product of static extents must be representable as index_type.");
----------------
Can this be moved to the other `static_assert` in this class?


================
Comment at: libcxx/include/__mdspan/layouts.h:17
+
+#ifndef _LIBCPP___MDSPAN_LAYOUTS_H
+#define _LIBCPP___MDSPAN_LAYOUTS_H
----------------
ldionne wrote:
> Let's move this to `libcxx/include/__fwd/mdspan_layouts.h`. While those are technically not forward declarations, they're close enough.
I would strongly suggest to use the name `libcxx/include/__fwd/mdspan.h` instead. The reason is the module logic of D144994 would need a special case for `mdspan_layouts.h` but works out of the box with `mdspan.h`


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

https://reviews.llvm.org/D151267