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

Thu Jul 20 13:37:28 PDT 2023

ldionne added inline comments.

================
Comment at: libcxx/include/CMakeLists.txt:478
   __locale
   __locale_dir/locale_base_api/bsd_locale_defaults.h
   __locale_dir/locale_base_api/bsd_locale_fallbacks.h
----------------
Let's add a release note and let's update the paper list.

================
Comment at: libcxx/include/__mdspan/mdspan.h:136
+                is_default_constructible_v<mapping_type> && is_default_constructible_v<accessor_type>)
+  = default;
+  _LIBCPP_HIDE_FROM_ABI constexpr mdspan(const mdspan&) = default;
----------------
crtrott wrote:
> ldionne wrote:
> > This raises an interesting question. The spec has preconditions (https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-2), but we can't check them if we make this `= default`. The `= default` is not required by the standard but it's arguably a nice property to have. This allows making `mdspan` trivially default constructible if the `data_handle_type`, `mapping_type` and `accessor_type` are trivially default constructible, right?
> > 
> > Actually, considering this a bit more, per https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-3 the effects are supposed to be: `Value-initializes ptr_, map_, and acc_.`. This is not the case if we make this `= default` and we have a `data_handle_type`, `mapping_type` or `accessor_type` that's an aggregate (not all of those might be possible but you get the point). So I think this is non-conforming. For example, if you have an aggregate `accessor_type` with a dummy `int` member, that `int` will never be value-initialized (zeroed-out) with your current implementation, but it should be according to the Standard. This needs a test.
> Doesn't the {} make it so its value initialized?
> 
> ```
>   _LIBCPP_NO_UNIQUE_ADDRESS data_handle_type __ptr_{};
>   _LIBCPP_NO_UNIQUE_ADDRESS mapping_type __map_{};
>   _LIBCPP_NO_UNIQUE_ADDRESS accessor_type __acc_{};
> ```
> 
> I believe this makes it so that the three members are value initialized for the defaulted default constructor doesn't it?
> 
> Which means the only thing we are not doing is the uncheckable precondition.
You're right, sorry I missed that.

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h:38
+// - does not check dynamic to static extent conversion in converting ctor
+// - check via sideeffects that mdspan::swap calls mappings swap via ADL
+
----------------

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h:49
+
+template <size_t _WrapArg>
+template <class _Extents>
----------------
Also no uglification in test code, since that makes the test code (pedantically) ill-formed for using reserved names.

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h:51
+template <class _Extents>
+class layout_wrapping_integral<_WrapArg>::mapping {
+  static constexpr typename _Extents::index_type _Wrap = static_cast<_Extents::index_type>(_WrapArg);
----------------
Would it make sense to use `layout_right::mapping` under the hood to implement this? It would reduce the complexity of this test class quite a bit, no?

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h:55-56
+public:
+  static_assert(std::__mdspan_detail::__is_extents<_Extents>::value,
+                "layout_left::mapping template argument must be a specialization of extents.");
+
----------------
This shouldn't be using any libc++ internal utilities since this is test code.

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h:65
+private:
+  _LIBCPP_HIDE_FROM_ABI static constexpr bool __required_span_size_is_representable(const extents_type& __ext) {
+    if constexpr (extents_type::rank() == 0)
----------------
We don't use these ABI macros in test code.

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/assert.conversion.pass.cpp:10
+// XFAIL: availability-verbose_abort-missing
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_HARDENED_MODE=1
+
----------------
For the assertion tests, we now use `// UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode`, and you can drop `ADDITIONAL_COMPILE_FLAGS`.

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/assign.pass.cpp:109
+int main(int, char**) {
+  (void)test();
+  static_assert(test());
----------------
This is a nitpick, but we usually just use `test();`, not `(void)test();`. Consider doing for consistency with the numerous other tests where we have that pattern.

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp:194
+
+  // possibility matrix for constructibility and convertibility https://godbolt.org/z/98KGo8Wbc
+  // you can't have convertibility without constructibility
----------------
This is crazy. If there was a test thoroughness award, you'd get it. Thanks!

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp:234
+
+  // Ran into trouble with doing it all in one static_assert: exceeding step limit for consteval
+  static_assert(test<t, t, t, t, t, t, t, t, std::default_accessor<float>>(std::default_accessor<float>()));
----------------
Is there a reason why you're not checking the `conv_test_accessor_c` versions at compile-time?

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_integers.pass.cpp:51
+
+  if constexpr (std::is_constructible_v<M, typename M::extents_type> && std::is_default_constructible_v<A>) {
+    if !consteval {
----------------
Whenever I see this kind of logic in tests, I feel like it would be too easy to make a mistake in the logic of the test and render the test useless. Instead, what would you think of passing whether you expect `H`, `M` and `A` to be constructible as bool template arguments to this function? Then this would be hardcoded at the point of call instead of figured out "automatically" by the test. This applies in a bunch of places.

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/index_operator.pass.cpp:42
+// Clang 15 and 16 do not support argument packs as input to operator []
+#if defined(__clang_major__) && __clang_major__ < 17
+template <class MDS>
----------------
Let's use `TEST_CLANG_VER`, it'll be easier to find in the future and remove the workaround. Also below (and maybe elsewhere, please check).

================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/types.pass.cpp:64
+        ((sizeof_dht + MDS::rank_dynamic() * sizeof_idx + align - 1) / align) * align;
+    LIBCPP_STATIC_ASSERT(sizeof(MDS) == expected_size);
+  }
----------------
Nice!

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

https://reviews.llvm.org/D154367