[libcxx-commits] [PATCH] D154367: [libc++] mdspan - implement mdspan class

[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 passing CI. Thank you for all the effort you've put into this feature!



================
Comment at: libcxx/test/std/containers/views/mdspan/extents/ctor_from_array.pass.cpp:86
+
+  // index_type is not nothrow constructable
+  static_assert(std::is_convertible_v<IntType, unsigned char>);
----------------
There's a few instances of this typo.


================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h:31
+
+// Layout that wraps indices to test some idiosyncratic behavior
+// - only accepts integers as indices
----------------
Please explain how the actual layout differs from `layout_right` and the other ones.


================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h:189
+private:
+  extents_type extents_{}; // exposition only
+};
----------------
`exposition-only` is relevant in the standard, not in our tests. I would remove this.


================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h:321
+private:
+  extents_type extents_{}; // exposition only
+};
----------------
Same here for `exposition-only`.


================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/assign.pass.cpp:12
+
+// constexpr mdspan(const mdspan&) = default;
+//
----------------
This synopsis is wrong, please go through them to make sure.


================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/assign.pass.cpp:34-39
+  // The defaulted assignment operator seems to be deprecated because:
+  //   error: definition of implicit copy assignment operator for 'checked_accessor<const double>' is deprecated
+  //   because it has a user-provided copy constructor [-Werror,-Wdeprecated-copy-with-user-provided-copy]
+  if constexpr (!std::is_same_v<A, checked_accessor<const double>>)
+    m = m_org;
+  // even though the following checks out:
----------------
I think you can get rid of this by using a `= default` copy assignment operator. In all cases I don't understand why we're basically not testing anything in that case.


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

https://reviews.llvm.org/D154367