- Notifications
You must be signed in to change notification settings - Fork803
P3663R3 Future-proofsubmdspan_mapping#8526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Fixes NB PL-009, US 66-117 (C++26 CD).
jwakely commentedNov 15, 2025
That comment is asking to rename two things, one of which was added by this paper. This commit doesn't address that comment. |
| %FIXME: this is in adifferent subclause but the paper has no title for it | ||
| template<class IndexType, size_t... Extents, class... Slices> | ||
| constexpr auto submdspan_canonicalize_slices(const extents<IndexType, Extents...>& src, | ||
| Slices... slices); | ||
| template<class IndexType, size_t... Extents, class... SliceSpecifiers> | ||
| constexpr auto submdspan_extents(const extents<IndexType, Extents...>&, SliceSpecifiers...); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| %FIXME: this is in adifferent subclause but the paper has no title for it | |
| template<classIndexType,size_t... Extents, class... Slices> | |
| constexpr auto submdspan_canonicalize_slices(const extents<IndexType, Extents...>& src, | |
| Slices... slices); | |
| template<class IndexType, size_t... Extents, class... SliceSpecifiers> | |
| constexpr auto submdspan_extents(const extents<IndexType, Extents...>&, SliceSpecifiers...); | |
| template<class IndexType, size_t... Extents, class... SliceSpecifiers> | |
| constexpr auto submdspan_extents(const extents<IndexType,Extents...>&, SliceSpecifiers...); | |
| //\ref{mdspan.sub.canonical},\tcode{submdspan} slice canonicalization | |
| template<class IndexType, size_t... Extents, class... Slices> | |
| constexpr auto submdspan_canonicalize_slices(const extents<IndexType, Extents...>& src, | |
| Slices... slices); |
| struct full_extent_t { explicit full_extent_t() = default; }; | ||
| inline constexpr full_extent_t full_extent{}; | ||
| %FIXME: this is in adifferent subclause but the paper has no title for it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please see suggestion below; thanks!
| %FIXME: WEIRD: the paper inconsistently uses math font and code font for the "S" | ||
| % that we are defining. | ||
| % To my understanding, we should aim to use code fonts for types, | ||
| % so this math font $S$ should be replaced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
P2630 and the status quo both use math fonts for both slice types and slices; see [mdspan.sub.overview] 2.3 and 2.4. P2630 predates pack indexing, so it uses math-fontsubmdspan, so it removed the
All this is saying that (a) math font has a precedent (whether or not you think it's "WEIRD" ; - ) ), and (b) we would welcome more consistency, whatever that might mean in terms of fonts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Right, the formatting in the paper is intended.
| %FIXME: the way I marked up this definition is almost certainly not the way it should be done | ||
| \defnx{\tcode{submdspan} slide type for \tcode{IndexType}}{\tcode{submdspan}!slice type} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure why you added extra text here. The phrase of power is "submdspan slice type forIndexType." We did not define "submdspan slice type" by itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The extra text is the index entry for the term being defined here. But the term is spelled wrong:
| %FIXME: the way I marked up this definition is almost certainly not the way it should be done | |
| \defnx{\tcode{submdspan}slide type for\tcode{IndexType}}{\tcode{submdspan}!slice type} | |
| %FIXME: the way I marked up this definition is almost certainly not the way it should be done | |
| \defnx{\tcode{submdspan}slice type for\tcode{IndexType}}{\tcode{submdspan}!slice type} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think {submdspan!slice type forIndexType} or {submdspan!slice type for index type} would be a better index entry. i.e. use the full term, not just part of it.
| %FIXME: Oxford comma | ||
| \tcode{$S$::extent_type} and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oxford comma is good, but editorial. (Just fix it if you see it.)
| %FIXME: Oxford comma | |
| \tcode{$S$::extent_type} and | |
| \tcode{$S$::extent_type}, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed, the FIXME comments just create more work than fixing it.
| \pnum | ||
| Given a signed or unsigned integer type \tcode{IndexType}, | ||
| a type $S$ is a | ||
| \defnx{canonical \tcode{submdspan} index type for \tcode{IndexType}}{\tcode{submdspan}!canonical index type} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure why you're adding extra text here. The phrase of power is "canonicalsubmdspan index type forIndexType." The words "forIndexType" are not optional.
| \pnum | ||
| Given a signed or unsigned integer type \tcode{IndexType}, | ||
| a type $S$ is a | ||
| \defnx{canonical \tcode{submdspan} slice type for \tcode{IndexType}}{\tcode{submdspan}!canonical slice type} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Again, not sure why you're adding extra text here.
| Given an object \tcode{e} of type \tcode{E} that is a specialization of \tcode{extents}, and | ||
| an object \tcode{s} of type \tcode{S} | ||
| that is a canonical \tcode{submdspan} slice type for \tcode{E::index_type}, | ||
| the \defnx{\tcode{submdspan} slice range of \tcode{s} for the $k^\text{th}$ extent of \tcode{E}}{\tcode{submdspan}!slice range} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Again, not sure what the extra text at the end is for.
| %FIXME: the trailing comma here makes no grammatical sense | ||
| \defnx{valid \tcode{submdspan} slice type for the $k^\text{th}$ extent of \tcode{E}}{\tcode{submdspan}!valid slice type}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- Please feel welcome to remove the trailing comma : - )
- Again, not sure what the extra text at the end of this line is for.
| \defnx{valid \tcode{submdspan} slice type for the $k^\text{th}$ extent of \tcode{E}}{\tcode{submdspan}!valid slice type}, | ||
| if \tcode{S} is a canonical slice type for \tcode{E::index_type}, and | ||
| for | ||
| %FIXME: this is weird ... what is "x"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"forE::static_extent()"
| if \tcode{S} is a canonical slice type for \tcode{E::index_type}, and | ||
| for | ||
| %FIXME: this is weird ... what is "x"? | ||
| %Maybe say "for a value x"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's not "a value," it's the valueE::static_extent().
| %FIXME: what is this section supposed to be called? | ||
| \rSec4[mdspan.sub.canonical]{\tcode{submdspan_extents} something something} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| %FIXME: what is this section supposed to be called? | |
| \rSec4[mdspan.sub.canonical]{\tcode{submdspan_extents} something something} | |
| \rSec4[mdspan.sub.canonical]{\tcode{submdspan} slice canonicalization} |
| \tcode{\exposid{first_}<IndexType, $k$>(slices...)} \tcode{+} | ||
| \tcode{indices[\placeholder{map-rank}[$k$]]}. | ||
| \tcode{decltype(canonical-slice<IndexType>(slices...[k]))} | ||
| is a valid submdspan slice type for the $k^\text{th}$ extent of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| is a valid submdspan slice type for the$k^\text{th}$ extent of | |
| is a valid`submdspan` slice type for the$k^\text{th}$ extent of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| is a valid submdspan slice type for the$k^\text{th}$ extent of | |
| is a valid\tcode{submdspan} slice type for the$k^\text{th}$ extent of |
| %FIXME: The paper cites "equals Extents::rank()" as the pre-existing wording. | ||
| %Investigate whether the change is still okay (which only affects the left side). | ||
| \tcode{sizeof...(SliceSpecifiers)} equals \tcode{sizeof...(Extents)}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Your suggested change is correct; thank you!
| %FIXME: The paper cites "equals Extents::rank()" as the pre-existing wording. | |
| %Investigate whether the change is still okay (which only affects the left side). | |
| \tcode{sizeof...(SliceSpecifiers)} equals\tcode{sizeof...(Extents)}. | |
| \tcode{sizeof...(SliceSpecifiers)} equals\tcode{sizeof...(Extents)}. |
| \tcode{SubExtents::rank()} equals the number of $k$ such that | ||
| $S_k$ does not model \tcode{\libconcept{convertible_to}<IndexType>}; and | ||
| \tcode{SubExtents::rank()} equals | ||
| \tcode{\exposid{MAP_RANK}(slices, Extents::rank())}; and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we have the same issue as above here withExtents::rank(). The existing text below says "for each rank index $k$ of \tcode{Extents}", so it may pay to introduce a name for the extents type. For example, we could change paragraph 1 like this
Let
Edenoteextents<IndexType, Extents...>, and letslicesbe the pack ....
and then useE throughout, e.g., in paragraph 2.
That being said, perhaps it's better just to merge the text as-is and then file an LWG issue with the above as a proposed fix. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That sounds too invasive to do as part of applying the paper.
| %FIXME: IMPORTANT: There is a closing parenthesis in the inserted wording, | ||
| %but no opening parenthesis. | ||
| %Did we mean to do: (1 + ...) / ... | ||
| % or: (1 + ... / ...) | ||
| \tcode{1 + ($\Sigma_k$::extent_type::value - 1) / $\Sigma_k$::stride_type::value}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for pointing this out! We meant to do1 + ((extent - 1) / stride). The plus 1 goes on the very outside and is not part of the division.
| %FIXME: IMPORTANT: There is a closing parenthesis in the inserted wording, | |
| %but no opening parenthesis. | |
| %Did we mean to do: (1 + ...) / ... | |
| % or: (1 + ... / ...) | |
| \tcode{1 + ($\Sigma_k$::extent_type::value - 1) /$\Sigma_k$::stride_type::value}, | |
| \tcode{1 + ($\Sigma_k$::extent_type::value - 1) /$\Sigma_k$::stride_type::value}, |
| %FIXME: is ISO going to like a bullet ending in a colon? | ||
| the following expression is well-formed and has the specified semantics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
%FIXME: is ISO going to like a bullet ending in a colon?
I don't know; does it? ; - )
| %FIXME: is ISO going to like a bullet ending in a colon? | |
| the following expression is well-formed and has the specified semantics: | |
| the following expression is well-formed and has the specified semantics. |
| %FIXME: the comma before "otherwise" seems wrong | ||
| \tcode{i...[MAP_RANK(valid_slices,$\rho$)]}, otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| %FIXME: the comma before "otherwise" seems wrong | |
| \tcode{i...[MAP_RANK(valid_slices,$\rho$)]}, otherwise. | |
| \tcode{i...[MAP_RANK(valid_slices,$\rho$)]} otherwise. |
| %FIXME: this doesn't make any sense. | ||
| %How can an lvalue like "s" be the specialization of a type? | ||
| if \tcode{s} is a specialization of \tcode{strided_slice} and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| %FIXME: this doesn't make any sense. | |
| %How can an lvalue like "s" be the specialization of a type? | |
| if\tcode{s} is a specialization of\tcode{strided_slice} and | |
| if the type of\tcode{s} is a specialization of\tcode{strided_slice} and |
| %FIXME: comma? | ||
| where \tcode{s} is \tcode{slices...[$k$]}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is a lot of punctuation in 6.1, so a semicolon felt right. It's fine if you would like to use a comma, though. That would probably call for 6.2 to become "stride() otherwise."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the suggestion is a comma between "is true" and "where s is ..."
| if | ||
| \begin{itemize} | ||
| \item | ||
| %FIXME: drive-by fix this broken parenthesis, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't see what you're talking about here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There's a pre-existing stray) in the line below, after1 which should be fixed.
In the paper you noted "[ Editor's note: Please note drive-by fix in 1.3. ]" for a different drive-by in the next subclause, but we didn't spot this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's not "the same mistake" though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh I see, it is the same mistake, all the way down in [mdspan.sub.map.rightpad]. I misread that as [mdspan.sub.map.right] whichalso has notes about drive-by fixes.
Again, just fix it - the FIXME comment is making more work for people.
| Let \tcode{index_type} be \tcode{typename Extents::index_type}. | ||
| \pnum | ||
| Let \tcode{slices} be the pack introduced by the following declaration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please see your above comment about ISO maybe not liking the colon here; thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The previous case was not introducing a list, it was just right at the end of a paragraph, where there was nothing following the colon. Here it seems fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, no ... it wasn't. The previous case was a valid use of a colon too, but the incoming paper was misinterpreted. I'll reply above ...
mhoemmen left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for doing the wording for this! : - )
| \item | ||
| otherwise, | ||
| the same type as the function's template argument \tcode{IndexType}; | ||
| \tcode{(is_convertible_v<decltype(std::move(ls))>, IndexType> && ...)} is \tcode{true}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like we need to escape& in\tcode.
| \tcode{(is_convertible_v<decltype(std::move(ls))>, IndexType>&& ...)} is\tcode{true}. | |
| \tcode{(is_convertible_v<decltype(std::move(ls))>, IndexType>\&\& ...)} is\tcode{true}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There's also a rogue> before the comma which is present in the incoming paper, but needs to be removed.
| \tcode{(is_convertible_v<decltype(std::move(ls))>, IndexType>&& ...)} is\tcode{true}. | |
| \tcode{(is_convertible_v<decltype(std::move(ls)), IndexType>\&\& ...)} is\tcode{true}. |
| \tcode{M::index_type} | ||
| if the function is a member of a class\tcode{M}, | ||
| the declaration\tcode{auto [...ls] = std::move(s);} is well-formed | ||
| for some object\tcode{s} of type $S$, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
S was not in math font in the paper, but I agree that it should be, as was done here.
| Given a signed or unsigned integer type \tcode{IndexType}, | ||
| a type $S$ is a | ||
| \defnx{canonical \tcode{submdspan} index type for \tcode{IndexType}}{\tcode{submdspan}!canonical index type} | ||
| if $S$ is either \tcode{IndexType} or \tcode{constant_wrapper<V>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if$S$ is either\tcode{IndexType} or\tcode{constant_wrapper<V>} | |
| if$S$ is either\tcode{IndexType} or\tcode{constant_wrapper<v>} |
| an object \tcode{s} of type \tcode{S} | ||
| that is a canonical \tcode{submdspan} slice type for \tcode{E::index_type}, | ||
| the \defnx{\tcode{submdspan} slice range of \tcode{s} for the $k^\text{th}$ extent of \tcode{E}}{\tcode{submdspan}!slice range} | ||
| of \tcode{e} is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You've got "extent ofE as part of the defined term, then "ofe" following it.
It's supposed to be "extent ofe" in the defined term (lowercase), and nothing following it.
| \item | ||
| if \tcode{S::offset_type} is a specialization of \tcode{constant_wrapper}, | ||
| then \tcode{S::offset_type::value} is less than or equal to $x$; | ||
| %FIXME: I'm pretty sure there needs to be some combining "or" whatever for these bullets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's a conjunction, they all need to be true.
| \pnum | ||
| \indexlibraryglobal{\exposid{MAP_RANK}}% | ||
| For a pack \tcode{p} and an integer $i$, | ||
| let \tcode{\exposid{MAP_RANK}(p, $i$)} be the number of elements \tcode{p...[$j$]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
\placeholder instead? This is likeINVOKE andITER_TRAITS
| for any rank index $k$ of \tcode{extents()}, then | ||
| let \tcode{offset} be a value of type \tcode{size_t} equal to | ||
| \tcode{(*this).required_span_size()}. | ||
| \tcode{operator().required_span_size()}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| \tcode{operator().required_span_size()}. | |
| \tcode{required_span_size()}. |
This isn't in the paper.
| %FIXME: drive-by fix this broken parenthesis, | ||
| %even though the authors didn't say pretty please like for the same mistake | ||
| %in [mdspan.sub.map.rightpad] :( | ||
| for each $k$ in the range \range{0}{SubExtents::rank() - 1)},\newline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| %FIXME: drive-by fix this broken parenthesis, | |
| %even though the authors didn't say pretty please like for the same mistake | |
| %in [mdspan.sub.map.rightpad] :( | |
| for each$k$ in the range\range{0}{SubExtents::rank() - 1)},\newline | |
| for each$k$ in the range\range{0}{SubExtents::rank() - 1},\newline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The paper requests a drive-by fix fromlayout_left tolayout_right here, but that was already done inf2b0254
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The same commit also fixed_rank torank_ as also requested by the paper.
| \tcode{SliceSpecifiers...[$k$]} is a unit-stride slicetype; and | ||
| \item | ||
| for each $k$ in the range | ||
| %FIXME: drive-by fix the broken parenthesis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The paper asked for that change to be made, and it was in the approved wording, so please just make the requested edit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The paper also asks for two drive-by fixes here, which were already done by82bf75b
| \begin{itemdecl} | ||
| submdspan_mapping(m, valid_slices...) | ||
| \end{itemdecl} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is an expression being shown for the preceding sentence "the following expression is well-formed and has the specified semantics:"
It's not the start of a new \itemdecl
I think it should be a codeblock environment instead ... although then it's a bit weird having theResult andReturns elements for an expression.
Fixes#8464
Fixes NB PL-009, US 66-117 (C++26 CD).
Alsofixescplusplus/papers#2292
Alsofixeshttps://github.com/cplusplus/nbballot/issues/695
Alsofixeshttps://github.com/cplusplus/nbballot/issues/818
Somehow relates tohttps://github.com/cplusplus/nbballot/issues/815 but I'm not sure how