[libcxx-commits] [PATCH] D157171: [libc++] mdspan - implement layout_stride

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__mdspan/layout_stride.h:163
+      _LIBCPP_ASSERT_UNCATEGORIZED(
+          ([&]<size_t... _Pos>(index_sequence<_Pos...>) {
+            // basically sort the dimensions based on strides and extents, sorting is represented in permute array
----------------
I would suggest pulling this into a `__bubble_sort` helper function and moving the comment there. That way, if we want to change the approach later on (e.g. use `std::sort` for real), it'll be easier. The code will also get a lot simpler here.


================
Comment at: libcxx/include/__mdspan/layout_stride.h:196
+                                          const array<_OtherIndexType, __rank_>& __strides) noexcept
+      : mapping(__ext, span(__strides)){};
+
----------------



================
Comment at: libcxx/include/__mdspan/layout_stride.h:230
+        ([&]<size_t... _Pos>(index_sequence<_Pos...>) {
+          return static_cast<index_type>(0) == static_cast<index_type>(__other((_Pos ? 0 : 0)...));
+        }(make_index_sequence<__rank_>())),
----------------
I think it would make sense to create an `__offset` function of some kind for https://eel.is/c++draft/mdspan.layout#stride.expo-2. It should handle the case where one of the extents is 0. Also probably needs a test.

Good catch!


================
Comment at: libcxx/include/__mdspan/layout_stride.h:284
+  // extents are zero.
+  // Technically its meaningless to query is_exhaustive() in that case, but unfortunately
+  // the way the standard defines this function, we can't give a simple true or false then.
----------------



================
Comment at: libcxx/include/__mdspan/layout_stride.h:313
+
+  // according to the standard layout_stride does not have a constraint on stride(r) for rank>0
+  // it still has the precondition though
----------------
This should be a LWG issue? It should be pretty simple.


================
Comment at: libcxx/include/__mdspan/layout_stride.h:325
+    if (([&]<size_t... _Pos>(index_sequence<_Pos...>) {
+          return static_cast<index_type>(__rhs((_Pos ? 0 : 0)...));
+        }(make_index_sequence<__rank_>())) != static_cast<index_type>(0))
----------------
This is `__offset` again. Is it wrong here too? If wrong, this is missing a test.


================
Comment at: libcxx/test/libcxx/containers/views/mdspan/layout_left/assert.ctor.layout_stride.pass.cpp:13
+// XFAIL: availability-verbose_abort-missing
+// ADDITIONAL_COMPILE_FLAGS: -Wno-ctad-maybe-unsupported
+
----------------
Can you link to the LLVM issue you created about this? We should have a way to do this with _LIBCPP_CTAD_SUPPORTED_FOR_TYPE.


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_right/ctor.layout_stride.pass.cpp:33-36
+template <class To, class From>
+constexpr void test_implicit_conversion(To dest, From src) {
+  assert(dest == src);
+}
----------------
That way you can't mess up the test by calling it with different arguments.


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

https://reviews.llvm.org/D157171