Extract submdspan_canonicalize_slices as standalone function (from PR #408)#451
Extract submdspan_canonicalize_slices as standalone function (from PR #408)#451
submdspan_canonicalize_slices as standalone function (from PR #408)#451Conversation
Co-authored-by: crtrott <9490481+crtrott@users.noreply.github.com>
submdspan_canonicalize_slices as standalone function (from PR #408)
| template<size_t k, class Slice, class IndexType, size_t... Extents> | ||
| MDSPAN_INLINE_FUNCTION | ||
| constexpr auto submdspan_canonicalize_one_slice( | ||
| [[maybe_unused]] const extents<IndexType, Extents...>& exts, | ||
| [[maybe_unused]] Slice s) | ||
| { |
There was a problem hiding this comment.
Neither exts, nor Extents, nor k is actually used in this function. Can we not simply template it on IndexType and Slice, and only have that single Slice argument passed in?
There was a problem hiding this comment.
Done in 68134d1. Simplified to template<class IndexType, class Slice> with a single Slice s argument — the unused k, Extents, and exts are all removed.
| const extents<IndexType, Extents...>& exts, | ||
| Slices... slices) | ||
| { | ||
| (check_submdspan_slice_mandate<Inds>(exts, get_kth_in_pack<Inds>(slices...)), ...); |
There was a problem hiding this comment.
Doesn't this work?
| (check_submdspan_slice_mandate<Inds>(exts, get_kth_in_pack<Inds>(slices...)), ...); | |
| (check_submdspan_slice_mandate<Inds>(exts, slices), ...); |
There was a problem hiding this comment.
Done in 68134d1. Updated to (check_submdspan_slice_mandate<Inds>(exts, slices), ...);
crtrott
left a comment
crtrott
left a comment
There was a problem hiding this comment.
Couple things there I think we could do it differently, which I think would eliminate the need for the pack extraction function.
…_in_pack Co-authored-by: crtrott <9490481+crtrott@users.noreply.github.com>
nmm0
left a comment
nmm0
left a comment
There was a problem hiding this comment.
A bunch of comments, I think it mostly looks good but I wanna do another pass after some discussion/changes
| ) | ||
| MDSPAN_INLINE_FUNCTION | ||
| constexpr auto canonical_ice([[maybe_unused]] S s) { | ||
| static_assert(std::is_signed_v<IndexType> || std::is_unsigned_v<IndexType>); |
There was a problem hiding this comment.
Did we want to preserve the comment here
| { | ||
| // Mandate checks (static_asserts only, no computation). | ||
| // Separated from canonicalization for clarity. | ||
| check_submdspan_slice_mandates( |
There was a problem hiding this comment.
I assume this was an ai-generated change -- I'm not sure it's clearer, and it may have some impact on compile time as you have to generate additional templates just for checking.
Since you are already processing one slice at a time I prefer the way it used to be with the mandate checking done on the slice
| constexpr auto submdspan_canonicalize_one_slice([[maybe_unused]] Slice s) | ||
| { | ||
| if constexpr (std::is_convertible_v<Slice, full_extent_t>) { | ||
| return full_extent; // canonical full-extent slice |
There was a problem hiding this comment.
idk if it matters as i don't quite get why it's stated in the standard but https://eel.is/c++draft/mdspan.sub#helpers-7 says this should be return static_cast<full_extent_t>(std::move(s))
| /* requires */ (std::is_convertible_v<S, IndexType>) | ||
| ) | ||
| MDSPAN_INLINE_FUNCTION | ||
| constexpr auto canonical_ice([[maybe_unused]] S s) { |
There was a problem hiding this comment.
Rename to canonical_index as in P3663?
| return cw<static_cast<IndexType>(index_cast<IndexType>(S::value))>; | ||
| } | ||
| else { | ||
| return static_cast<IndexType>(index_cast<IndexType>(s)); |
There was a problem hiding this comment.
Another slight difference with the standard, should this be std::move(s)? I'm not 100% what the purpose of that is unless there is an index type that is movable...
| #endif | ||
| else { | ||
| // General pair-like case: structured binding into [first, last) | ||
| auto [s_k0, s_k1] = s; |
There was a problem hiding this comment.
Another std::move(s) from the standard
| constexpr check_static_bounds_result check_static_bounds( | ||
| const extents<IndexType, Exts...>&) | ||
| { | ||
| #if defined(__cpp_pack_indexing) |
There was a problem hiding this comment.
TBH I would rather just define this using index sequence while obvious pack indexing is much preferable it's C++26 so in most cases we will have to look at the alternate code.
This is especially true since we are using an index sequence to generate this anyway, so imo lets just pass down the indices
| } (); | ||
| #endif | ||
|
|
||
| if constexpr (std::is_convertible_v<S_k, full_extent_t>) { |
There was a problem hiding this comment.
This is clearly true but for the life of me I can't figure out where in the standard it says full extent is always a valid slice type (I looked in) https://eel.is/c++draft/mdspan.sub#overview-8
| } | ||
| else if constexpr (std::is_convertible_v<S_k, IndexType>) { | ||
| if constexpr (is_integral_constant_like_v<S_k>) { | ||
| if constexpr (de_ice(S_k{}) < 0) { |
There was a problem hiding this comment.
Is this needed? I thought you could compare integral constants to integers
| auto get_first = [] (S_k s_k) { | ||
| auto [s_k0, _x] = s_k; | ||
| return s_k0; | ||
| }; | ||
| auto get_second = [] (S_k s_k) { | ||
| auto [_x, s_k1] = s_k; | ||
| return s_k1; | ||
| }; |
There was a problem hiding this comment.
Does this not work:
auto [first, second] = s_k;
And just use those?
submdspan_canonicalize_slicesimplementationinclude/experimental/__p2630_bits/submdspan_canonicalize_slices.hpptests/test_canonicalize_slices.cppwith 10 teststests/CMakeLists.txtandsubmdspan.hppsubmdspan_canonicalize_one_slice: simplified totemplate<class IndexType, class Slice>— removed unusedk,Extents, andextsparameterscheck_submdspan_slice_mandates: use direct parallel pack expansion(check_submdspan_slice_mandate<Inds>(exts, slices), ...)instead ofget_kth_in_packsubmdspan_canonicalize_slices_impltuple return: usesubmdspan_canonicalize_one_slice<IndexType>(slices)...instead ofget_kth_in_packget_kth_in_packhelper (no longer needed)✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.