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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

I have a few more comments but this pretty much LGTM assuming CI passes.



================
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
+
----------------
crtrott wrote:
> ldionne wrote:
> > 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.
> https://github.com/llvm/llvm-project/issues/64719
Sorry, I meant add a comment in the test so that we can remove the `-Wno-ctad-...` at some point.


================
Comment at: libcxx/test/libcxx/containers/views/mdspan/layout_right/assert.ctor.layout_stride.pass.cpp:62
+  }
+  // strides are not layout_left compatible
+  {
----------------
Is this what you mean or is the test wrong?


================
Comment at: libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.index_operator.pass.cpp:11
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// UNSUPPORTED: !libcpp-hardening-mode=debug
+// XFAIL: availability-verbose_abort-missing
----------------
Shouldn't these assertions be enabled in the hardened mode too?


================
Comment at: libcxx/test/std/containers/views/mdspan/layout_left/ctor.layout_stride.pass.cpp:55
+  if constexpr (implicit) {
+    dest = src;
+    assert(dest == src);
----------------
I don't understand why you are performing an assignment here. Aren't we trying to test construction?

Also, the following should be sufficient to check for implicit constructibility:

```
To dest = src;
```

this should allow getting rid of `test_implicit_conversion`. Yes, I left a comment last time to change to the lambda but I guess I had not realized you could simplify this even more altogether.


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

https://reviews.llvm.org/D157171