View-Refactor: mdspan implementation updates #7288
Conversation
| @@ -17,6 +17,7 @@ | |||
| #pragma once | |||
|
|
|||
| #include <tuple> | |||
| #include <complex> | |||
There was a problem hiding this comment.
Are you sure you want/need to include <complex> here ? It's surprising.
There was a problem hiding this comment.
There was a problem hiding this comment.
But isn't this pair-like thingy used for slicing ? If so, I would be surprised that slicing is allowed with a complex... I can't image how bad a code would be if complex numbers were used for that.
There was a problem hiding this comment.
turns out that the ISO C++ standard defines the exposition only concept pair-like which is defined through tuple-like which is defined here: https://eel.is/c++draft/tuple.like#concept:tuple-like
So complex<double> is a valid slice specifier for submdspan because its pair-like and double is convertible to index_type
There was a problem hiding this comment.
It is a draft right ? If so I would refrain supporting such a ill-formed case.
There was a problem hiding this comment.
no this is final. This was discussed in the committee and it is basically not avoidable. We want "convertible to index_type" to be valid, that means floating points will be valid. And the committee did not feel like adding an explicit "convertible to index_type but not floating point" to the specification of all these things in mdspan.
There was a problem hiding this comment.
OK, then I wouldn't go against the committee.
But in this specific case, you're still including complex in a very central file, so any downstream project using Kokkos would have complex included ?!
There was a problem hiding this comment.
That is already the case, because you get it indirectly through Kokkos_Complex
There was a problem hiding this comment.
I still think that Kokkos should not pull <xomplex> for me, but do what you think is best. I won't block for this of course 😉
| @@ -95,6 +95,7 @@ struct padded_extent { | |||
| using static_array_type = typename static_array_type_for_padded_extent< | |||
| padding_value, _Extents, _ExtentToPadIdx, _Extents::rank()>::type; | |||
|
|
|||
| MDSPAN_INLINE_FUNCTION | |||
There was a problem hiding this comment.
inline is implicitly implied I think. Would be good to remove that useless specifier if so.
There was a problem hiding this comment.
There was a problem hiding this comment.
My 2 cents is that doing things "for code style consistency" is a bad practice IMO. In this case, stripping out INLINE makes the code shorter and more to the point, and remove a clearly useless specifier.
But do whatever you want 😄
Obviously I intended it to be and it is, but isn't checking this part of the review ;-) |
|
(could not think of a better way to trigger the CI and it did not work...) |
|
Obviously there is a CI SKIP directive... |
|
Should the mdspan interop updates get a changelog entry/section? |
This is a piece of #7286 fixing a few small mdspan things. I will open a PR on the mdspan implementation shortly also attempting to write a test for one of the conversion issues exposed and fixed here. (
padded_stride_type::init_paddingtakes anextents_typeas argument, which previously would have requiredother_mapping.extents()to be convertible toextents_type. The function constraint is only "is_constructible" though, so we need to explicitly cast before calling the helper.MDSpan PR here: kokkos/mdspan#358