This adds the mdspan class and its tests.
Details
Diff Detail
Unit Tests
Event Timeline
This implements about 80% of the necessary tests, but its already a lot of code so we can collect review feedback while we do the rest.
libcxx/include/__mdspan/mdspan.h | ||
---|---|---|
62 | _Uglification! | |
116 | ||
131–132 | libc++ doesn't work with MSVC anyways, so this workaround can be removed. | |
257 | These should be _LIBCPP_NO_UNIQUE_ADDRESS. | |
275 | ||
276 | ||
279 | ||
282 | ||
libcxx/test/std/containers/views/mdspan/mdspan/CustomTestAccessors.h | ||
2 | Missing license header and include guard |
libcxx/include/__mdspan/mdspan.h | ||
---|---|---|
59–71 | This can yield false positives if a type is both a mapping and a layout. It can actually lead to hard compiler errors if instantiating the nested types failed in a non SFINAE friendly way. This is a bit pedantic, but I'd be a lot more comfortable if we only restricted this to catching types that we know are *definitely* mappings, i.e. the mappings we provide. We could do that by defining the __libcxx_is_mapping nested type inside our mappings, and then checking: template <class _Tp> concept __is_definitely_mapping = requires { typename _Tp::__libcxx_is_mapping; }; Then we assert !__is_definitely_mapping<_LayoutPolicy> instead of !__is_possibly_mapping<_LayoutPolicy>. If this isn't harsh enough of a diagnostic cause it misses user-defined policies, I'd like to see this as an incremental change later, but not as part of the initial design of this class. BTW we have a precedent for this with __is_join_view_iterator. Edit: Instead, I think we could do this too: template <class _Layout, class _Extents> concept __has_invalid_mapping = requires !requires { typename _Layout::template mapping<_Extents>; }; Then from within mdspan, you could do: static_assert(!__has_invalid_mapping<_LayoutPolicy>, "mdspan: did you pass a valid LayoutPolicy to mdspan? A common mistake is passing a mapping instead of a layout policy"); WDYT? This keeps the guard rails for user-defined layouts and mappings as well. And there are no possibilities for false positives AFAICT since _Layout::template mapping<_Extents> *has* to be valid. |
libcxx/include/__mdspan/mdspan.h | ||
---|---|---|
66 | Do you test that the default layout policy is layout_right and the default accessor policy is default_accessor<_ElementType>? You can do that with static_assert(std::is_same_v<mdspan<T, E>, mdspan<T, E, layout_right, default_accessor<T>>>); Should probably be in types.pass.cpp if not already there. | |
131–132 | Not sure what this comment refers to, but the nvhpc comment should be removed. | |
137 | 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. | |
138–139 | Do you have tests that those are trivial operations if the members are trivially copy/move constructible? | |
158 | There's a few instances of that. | |
159 | These requirements are wrong AFAICT. They should be using const& like the ones for array as described in https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-8.1. I think what you want is is_convertible_v<const _OtherIndexType&, index_type> && is_nothrow_constructible_v<index_type, const _OtherIndexType&>. Those need tests. If you have an index type where index_type(_OtherIndexType&&) works but index_type(_OtherIndexType const&) doesn't, then the requires before my comment will be satisfied but you will get a hard compiler error while trying to compile __map_(extents_type(__exts)) below (I think). Please also add the same test for the std::array constructor above: if I go and remove const& from the equivalent requires above, a test should fail. | |
164 | If you forget to std::move here (or in any of the similar constructors), which test fails? If there's none, please make sure to add coverage. | |
167 | This needs a test, which should be possible if you have an extents_type where mapping_type(extents_type&&) works but mapping_type(extents_type const&) doesn't. Then the requires before my comment would enable this mdspan constructor, but you'd get a hard compiler error while trying to compile __map_(__exts) below. | |
178–179 | Needs tests. | |
185–186 | ||
191 | ||
193–194 | ||
205–206 | Same question about testing for triviality. | |
209 | These "stable" names seem to originate from a previous version of the standard. https://eel.is/c++draft/views.multidim#mdspan doesn't seem to have the same names. | |
214 | Just for consistency with the spec. | |
216 | We have an open patch to categorize those. | |
222 | Needs a test. The span version has the same problem. | |
224 | Is there any reason why we're doing this here instead of following the standard wording more closely? Is it because you want to avoid calling the variadic version in case you have a large number of dimensions? I would inline all the uses of __idx_fold into their calling functions and use a lambda instead, that's easier to read. Otherwise the implementation of the function is basically 100+ lines above the function itself. | |
230 | Note to self: I have not actually reviewed the implementation of these idx_fold operations yet. | |
libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_array.pass.cpp | ||
49–52 | I don't think that works. The usual pattern we use here is template <class T> void accept(T); template <class MDS, class Exts> concept check_mdspan_ctor_implicit = requires(typename MDS::data_handle_type h, const Exts& exts) { accept<MDS>({h, exts}); }; You'll want to make all of your explicit tests consistent. | |
libcxx/test/std/containers/views/mdspan/mdspan/properties.pass.cpp | ||
99 | Those static functions should be assert(...)ed instead. Then we call those from a constexpr and a non-constexpr context. Right now the following implementation passes your tests: static constexpr rank_type rank() noexcept { if (std::is_constant_evaluated()) return extents_type::rank(); else return 0; } I think all the "properties" are affected. |
Addressed all the comments, uploading patch now.
libcxx/include/__mdspan/mdspan.h | ||
---|---|---|
66 | Added. | |
137 | 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. | |
138–139 | Adding it. | |
159 | index_type is integral so I don't think we can write a test for that using the move thing? I.e. the only thing you a move into int is an int. However, we could test that we have an OtherIndexType where the conversion operator to integral is non-const and/or missing no-except. Turns out that this was already wrong in extents itself, which I need to fix in order to test mdspan, because otherwise it can go through the constructor taking extents ... | |
164 | Added coverage via move_counted_handle i.e. a handle type which increments a static variable during move. | |
167 | Fixed and added test by modifying the layout_wrapped_integral depending on value of the _Wrap it now has a constructor from extents_type.&& Note that construction from extents and construction from array/span/integers has different requirements for the mapping. We are now testing both. | |
178–179 | Added a whole bunch of testing, and made sure (i.e. tried) that any combination of missing const& both for the requires and the explicit will result in failure. | |
224 | Yeah, we can't avoid having an expansion happening in this operator via idx_fold (or now changed via a lambda) but we don't have to instantiate another variadic function here. We have ongoing issues with inlining depth in our big applications, and so we make judgement calls regarding delegation. This is the lambda version body of the function (a bit more than the fold expression but ok: return __acc_.access(__ptr_, [&]<size_t ... _Idxs>(index_sequence<_Idxs...>) { return __map_(__indices[_Idxs]...); }(make_index_sequence<rank()>())); This is the forwarding version: return [&, self = this]<size_t ... _Idxs>(index_sequence<_Idxs...>) -> reference { return (*self)[__indices[_Idxs]...]; }(make_index_sequence<rank()>()); I personally don't see that as better, we need to explicitly capture this, and need to define the return type (otherwise it returns a value not a reference. | |
230 | Removed it. | |
libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_array.pass.cpp | ||
49–52 | I don't think that works, because it's multi argument explicitlness. I.e. this doesn't even compile: class Bar { constexpr Bar(int, int) {} }; template<class T> constexpr bool accept(T) { return true; } static_assert(accept({1,1})); My understanding is that for multi argument constructors the ONLY difference in whether you have an explicit or not is whether T a = {....}; works. It doesn't make any difference for function calls. | |
libcxx/test/std/containers/views/mdspan/mdspan/properties.pass.cpp | ||
99 | Yeah done. |
This addresses Louis review comments. Most notably fixing a lot of constraints regarding const& vs non-const arguments and adding testing for that.
libcxx/include/CMakeLists.txt | ||
---|---|---|
479 | Let's add a release note and let's update the paper list. | |
libcxx/include/__mdspan/mdspan.h | ||
137 | You're right, sorry I missed that. | |
libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h | ||
39 | ||
50 | Also no uglification in test code, since that makes the test code (pedantically) ill-formed for using reserved names. | |
52 | 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? | |
56–57 | This shouldn't be using any libc++ internal utilities since this is test code. | |
66 | We don't use these ABI macros in test code. | |
libcxx/test/std/containers/views/mdspan/mdspan/assert.conversion.pass.cpp | ||
11 | For the assertion tests, we now use // UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode, and you can drop ADDITIONAL_COMPILE_FLAGS. | |
libcxx/test/std/containers/views/mdspan/mdspan/assign.pass.cpp | ||
110 | 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. | |
libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp | ||
195 | This is crazy. If there was a test thoroughness award, you'd get it. Thanks! | |
235 | Is there a reason why you're not checking the conv_test_accessor_c versions at compile-time? | |
libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_integers.pass.cpp | ||
52 | 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. | |
libcxx/test/std/containers/views/mdspan/mdspan/index_operator.pass.cpp | ||
43 | 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). | |
libcxx/test/std/containers/views/mdspan/mdspan/types.pass.cpp | ||
65 | Nice! |
libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h | ||
---|---|---|
52 | I looked at it, because we wanted a layout which is runtime conditionally non-unique and not-strides, almost all the functions are different. So nothing to be saved really. | |
libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_integers.pass.cpp | ||
52 | OK done. | |
52 | I did it for the ones where it seemed straight forward, and did not do it for conversion, where its kinda combinatorial what happens. That thing has more local checks to assure that we test what we believe we test. |
libcxx/test/std/containers/views/mdspan/mdspan/index_operator.pass.cpp | ||
---|---|---|
43 | TEST_CLANG_VER is not defined on apple clang so we would need to check for TEST_CLANG_VER and TEST_APPLECLANG_VER, clang-major works universally here. |
Trigger all mdspan assertions in hardened mode: all of them are related in one form or another to bounds checking. Either directly, or in order to not screw up the bounds during construction/assignment/conversion etc. which would violate the invariants of mdspan, and thus could lead to bounds checks not triggering when they should.
LGTM with passing CI. Thank you for all the effort you've put into this feature!
libcxx/test/std/containers/views/mdspan/extents/ctor_from_array.pass.cpp | ||
---|---|---|
86 | There's a few instances of this typo. | |
libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h | ||
32 | Please explain how the actual layout differs from layout_right and the other ones. | |
190 | exposition-only is relevant in the standard, not in our tests. I would remove this. | |
322 | Same here for exposition-only. | |
libcxx/test/std/containers/views/mdspan/mdspan/assign.pass.cpp | ||
13 | This synopsis is wrong, please go through them to make sure. | |
35–40 | 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. |
libcxx/include/__mdspan/extents.h | ||
---|---|---|
178 | I would keep this _LIBCPP_ASSERT_UNCATEGORIZED and then @var-const can categorize them appropriately. |
libcxx/include/__mdspan/extents.h | ||
---|---|---|
210 | Can you please double-check if this assertion is tested? | |
libcxx/test/std/containers/views/mdspan/layout_right/assert.conversion.pass.cpp | ||
1 ↗ | (On Diff #542988) | Nit: I think tests that check assertions usually are under libcxx/test/libcxx, not libcxx/test/std since they test libc++-specific behavior. This can be done in a follow-up. |
libcxx/test/std/containers/views/mdspan/mdspan/assign.pass.cpp | ||
---|---|---|
13 | sorry accidentally published that above comment: it was wrong (copy ctor instead of copy assign) and I fixed it |
libcxx/include/__mdspan/extents.h | ||
---|---|---|
210 | Checked in libcxx/test/std/containers/views/mdspan/extents/assert.obs.pass.cpp |
I investigated the failure on MinGW and tracked it down to a reproducer on godbolt, which indicates this is a bug in the compiler when using --target=x86_64-w64-windows-gnu
https://godbolt.org/z/Y6Mb7sMMc
Shortish version, you have something like this:
template<class T, class M, class A> class mdspan { public: mdspan() = default; mdspan(const mdspan&) = default; mdspan(T* ptr_, M map_, A acc_):ptr(ptr_),map(map_),acc(acc_) {} template<class OT, class OM, class OA> mdspan(const mdspan<OT, OM, OA>& other):ptr(other.ptr), map(other.map), acc(other.acc) {} [[no_unique_address]] T* ptr; [[no_unique_address]] M map; [[no_unique_address]] A acc; };
If you now do mdspan<const T, M2, A2>(mdspan<T, M1, A1>); where M1, A1 and A2 are empty, but M2 is not, and A1->A2 happens via a conversion operator in A1, not a converting constructor in A2 the last byte of ptr gets zeroed out.
The only standard conformant way to fix this is to not have [[no_unique_address]].
However this is a rather esoteric corner case, where you would need to have two accessors where A2 is constructible from A1, but doesn't have a constructor accepting const A1&.
So couple options:
a) I just don't test that combo on MinGW, and users could run into this behavior which we will report as a bug, if they do something weird.
b) we remove [[no_unique_address]] for now on mdspan for a MinGW build, and break ABI later if and when this bug gets fixed
c) I don't land mdspan today, and we hope the bug is fixed in MinGW with clang 17.
I personally would opt for a) since the chances users will run into this are virtually zero, and one can work around this bug.
libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp | ||
---|---|---|
241 | Please add a link to the bug you filed. |
Uploaded the wrong patch file last time around, where I guarded the wrong two tests on MinGW :-(
This has landed as 488c3db245ce0fe3b70357d6798ee91c5baa82a2 in main and 9957225e7d6761d7e550561eac1df78e5e2c184b in LLVM-17 release branch.
Let's add a release note and let's update the paper list.