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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__mdspan/extents.h:459
+  requires(is_integral_v<_From>)
+_LIBCPP_HIDE_FROM_ABI constexpr bool __is_multidimensional_index_in(_IndexType __extent, _From __value) {
+  if constexpr (is_signed_v<_From>) {
----------------
`__is_index_in_extent`


================
Comment at: libcxx/include/__mdspan/extents.h:480
+_LIBCPP_HIDE_FROM_ABI constexpr bool
+__is_multidimensional_index_in(index_sequence<_Idxs...>, _Extents __ext, _From... __values) {
+  return (__mdspan_detail::__is_multidimensional_index_in(__ext.extent(_Idxs), __values) && ...);
----------------
Either rename to `_impl` or inline it via a lambda below.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:17
+
+#ifndef _LIBCPP___MDSPAN_LAYOUT_RIGHT_MAPPING_H
+#define _LIBCPP___MDSPAN_LAYOUT_RIGHT_MAPPING_H
----------------
Let's make this `__mdspan/layout_right.h` since we're basically implementing the whole class here even though we have a "fwd decl" somewhere else.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:28
+#include <__utility/integer_sequence.h>
+#include <__utility/unreachable.h>
+#include <cinttypes>
----------------
Not necessary anymore?


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:46-47
+class layout_right::mapping {
+  static_assert(__mdspan_detail::__is_extents<_Extents>::value,
+                "layout_right::mapping template argument must be a specialization of extents.");
+
----------------
Do you have a `.verify.cpp` test for this diagnostic?

I think you want something like:

```
//expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}MESSAGE}}
```

see `libcxx/test/libcxx/algorithms/bad_iterator_traits.verify.cpp` for example.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:50-54
+  using extents_type = _Extents;
+  using index_type   = typename extents_type::index_type;
+  using size_type    = typename extents_type::size_type;
+  using rank_type    = typename extents_type::rank_type;
+  using layout_type  = layout_right;
----------------
Note to self: those are tested (thanks!).


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:57
+private:
+  _LIBCPP_HIDE_FROM_ABI static constexpr bool __required_span_size_is_representable(extents_type __ext) {
+    if constexpr (extents_type::rank() == 0)
----------------
I don't think we need to make a copy here.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:63-66
+    for (rank_type __r = 1; __r < extents_type::rank(); __r++) {
+      if (__prod > (__num_max / __ext.extent(__r)))
+        return false;
+      __prod *= __ext.extent(__r);
----------------
I think we could do this instead:

```
index_type __prod              = __ext.extent(0);
for (rank_type __r = 1; __r < extents_type::rank(); __r++) {
    bool __overflowed = __builtin_mul_overflow(__prod, __ext.extent(__r), &__prod);
    if (__overflowed)
        return false;
}
```

This would avoid the division.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:78
+  _LIBCPP_HIDE_FROM_ABI constexpr mapping(const mapping&) noexcept = default;
+  _LIBCPP_HIDE_FROM_ABI constexpr mapping(const extents_type& __ext) noexcept : __extents_(__ext) {
+    _LIBCPP_ASSERT(__required_span_size_is_representable(__ext),
----------------
Do you have a test for the `noexcept`-ness? Edit: Yes.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:84
+  template <class _OtherExtents>
+    requires(is_constructible_v<extents_type, _OtherExtents>)
+  _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherExtents, extents_type>)
----------------
Do you have a test that checks that `mapping` is not constructible (using a type trait) if the extents are not constructible? This is important to make sure that you implemented this using `requires` (or `enable_if`) as opposed to it just being a hard error.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:116
+
+private:
+  // Helper functions to compute the offset
----------------
Can we move this `private` block to the end of the class? I usually don't mind implementation details in the middle of the class, but this one is quite large so it obscures the public API a bit too much.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:150
+  template <class... _Indices>
+    requires((sizeof...(_Indices) == extents_type::rank()) && (is_convertible_v<_Indices, index_type> && ...) &&
+             (is_nothrow_constructible_v<index_type, _Indices> && ...))
----------------
Please make sure you have SFINAE (or better, concepts) tests for those requirements.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:155
+                   "layout_right::mapping: out of bounds indexing");
+    return __compute_offset(__rank_count<0, extents_type::rank()>(), static_cast<index_type>(__idx)...);
+  }
----------------
```
return [&]<size_t ..._Pos>(index_sequence<_Pos...>) {
  index_type __result = 0;
  ((result = __idx + __extents_.extent(_Pos) * __result))...; // exercise for the reader :-)
  return __result;
}(index_sequence_for<_Indices...>{});
```



================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:170
+    _LIBCPP_ASSERT(__r < extents_type::rank(), "layout_right::mapping::stride(): invalid rank index");
+    if constexpr (extents_type::rank() > 0) {
+      index_type __s = 1;
----------------
This is always true, so can be removed.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:176
+    }
+    __libcpp_unreachable();
+  }
----------------
And this isn't necessary anymore either.


================
Comment at: libcxx/include/__mdspan/layout_right_mapping.h:180
+  template <class _OtherExtents>
+    requires(_OtherExtents::rank() == extents_type::rank())
+  _LIBCPP_HIDE_FROM_ABI friend constexpr bool
----------------
Can you make sure you have a test that we SFINAE away when the ranks are different?


================
Comment at: libcxx/include/__mdspan/layouts.h:17
+
+#ifndef _LIBCPP___MDSPAN_LAYOUTS_H
+#define _LIBCPP___MDSPAN_LAYOUTS_H
----------------
Let's move this to `libcxx/include/__fwd/mdspan_layouts.h`. While those are technically not forward declarations, they're close enough.


================
Comment at: libcxx/include/__mdspan/layouts.h:48-55
+// Will be implemented with follow on revision
+#  if 0
+// Layout policy with a unique mapping where strides are arbitrary
+struct layout_stride {
+  template<class Extents>
+    class mapping;
+};
----------------
+1 let's remove those until they are implemented, or make them a comment.


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_right/conversion.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Let's name this `ctor.mapping.pass.cpp`


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_right/ctor_default.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Let's use `ctor.default.pass.cpp`, it's more consistent with what we're doing recently.


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_right/ctor_extents.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
`ctor.extents.pass.cpp`


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_right/properties.pass.cpp:36
+template <class E>
+void test_layout_mapping_right() {
+  using M = std::layout_right::template mapping<E>;
----------------
Please use a classic `constexpr` test here -- I'd like to make sure that these can also be called at runtime.

For example, your test (as currently written) would pass if we did something like:

```
static constexpr bool is_always_unique() noexcept {
  if (!std::is_constant_evaluated())
    throw "ahah";
  return true;
}
```



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

https://reviews.llvm.org/D151267