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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with a few comments! Thanks!



================
Comment at: libcxx/include/__mdspan/layout_right.h:107
+
+  _LIBCPP_HIDE_FROM_ABI constexpr index_type required_span_size() const noexcept {
+    index_type __size = 1;
----------------
This doesn't seem to be tested directly.


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_right/ctor.default.pass.cpp:33-34
+
+  // NOTE should we be passing it in instead
+  // check required_span_size()
+  typename E::index_type expected_size = 1;
----------------
You should either address this or remove the comment.


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_right/ctor.default.pass.cpp:38
+    expected_size *= e.extent(r);
+  ASSERT_NOEXCEPT(m.required_span_size());
+  assert(m.required_span_size() == expected_size);
----------------
This would belong to the direct test for `required_span_size()` instead, for example.


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_right/ctor.mapping.pass.cpp:79
+
+  // Sanity check that one static to dyamic conversion works
+  static_assert(std::is_constructible_v<mapping_t<int, D>, mapping_t<int, 5>>);
----------------
(maybe elsewhere if you copy-pasted, just grep)


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_right/index_operator.pass.cpp:31
+
+struct IntType {
+  int val;
----------------
Let's use `libcxx/test/std/containers/views/mdspan/extents/ConvertibleToIntegral.h` (which you can move to `libcxx/test/std/containers/views/mdspan/` instead).


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_right/index_operator.pass.cpp:86-90
+  test_iteration<std::extents<int>>();
+  test_iteration<std::extents<unsigned, D>>(7);
+  test_iteration<std::extents<unsigned, 7>>();
+  test_iteration<std::extents<unsigned, 7, 8>>();
+  test_iteration<std::extents<int64_t, D, 8, D, D>>(7, 9, 10);
----------------
Let's test with a few 1-sized dimensions.


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

https://reviews.llvm.org/D151267