The implementation looks solid now, only very small comments there. Please add a patch updating feature test macro, my preference would be to keep internal padded layouts FTM and give it a value 202403, with comment that corresponds to new layouts part of https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p2642r6.pdf.
I assume this is because, you were unsure about the value of FTM, fortunately sd-6 ( https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations ) already list the value for paper 202411. Regards, Tomasz On Fri, Dec 5, 2025 at 10:29 AM Tomasz Kaminski <[email protected]> wrote: > > > On Thu, Dec 4, 2025 at 10:27 PM Luc Grosheintz <[email protected]> > wrote: > >> These are the revised submdspan_mapping related patches from v2. >> > I will try to review it fully today. > >> >> A few comments: >> >> - __any_past_the_end: the suggestion was to precheck for an empty >> extent. I kept the current logic, because the additional complexity >> is only at compile time (there was a `constexpr` missing on a `else >> if` branch in v2). At runtime, it's a very simple check: >> (__slice_begin[k] == exts.extent[k]) || ... >> Even if the pre-check would make the condition simpler, for regular >> (non-empty) cases we'd be forced to first check for empty, then >> check for out-of-bounds. Unless I'm missing something, that's >> strictly worse. >> > Yes, after some time of break I think you are making the right call here. > >> >> The main motivation here is to eliminate these checks when the slice >> is a collapsing slice, because these checks adds a "lot" of code >> compared to the hand-rolled version. (With the current >> implementation: submdspan(m, i, j, k) and mdspan(&m[i, j, k], E{}) >> generate the same code, with E = std::extents<int>). >> >> - As proposed during review, the code to figure out the layout mapping >> type given a combination of slices has been rewritten. Essentially, >> we have a `enum class _SliceKind` what's (forced to be) categorical. >> From there one can use for-loops and immediate (consteval) functions >> insteads of traditional recursive structs. >> > It looks much more readable now, only leaving small comments about not > using > _Size as template parameter. > >> >> The twist here is that a __full slice is not a __unit_strided_slice >> slice. This is because: I don't want to use weak/plain enums; >> strong enums (enum class) don't do bitwise operations without >> implementing a lot of boilerplate; bitset feels inappropriate. >> Hence, this solution felt the one with the least boilerplate. >> Again, I might be missing a trick. >> >> - In __substrides_standardized I chose not use _M_strides, because >> _M_strides has size `rank`; while __substrides_standardized only >> returns an array of lenght `__subrank`. The difference can be seen, >> e.g., for submdspan(m, 1, 2, 3, full) which (technically) performs 3 >> multiplications when zero are needed. Therefore, one would rely on >> the optimizer to eliminate the extra operation. It might be >> recognizable as dead code, but I've not checked. >> > Yes, it makes sense. >> >> >> - Let me know if you want me to move the __subextents code in a >> separate commit. >> > Not necessary. > >> >> - I started following the suggestion to always use qualified names for >> functions. Let me know if you want to qualify everything (typenames, >> concepts, etc.) >> > We qualify functions to avoid the compiler doing ADL, even if it will > result in only a > single candidate. No need to qualify anything else. > >> >> Luc Grosheintz (5): >> libstdc++: Implement submdspan and submdspan_mapping for layout_left. >> [PR110352] >> libstdc++: Implement submdspan_mapping for layout_right. [PR110352] >> libstdc++: Implement submdspan_mapping for layout_stride. [PR110352] >> libstdc++: Implement submdspan_mapping for layout_left_padded. >> [PR110352] >> libstdc++: Implement submdspan_mapping for layout_right_padded. >> [PR110352] >> >> libstdc++-v3/include/std/mdspan | 713 ++++++++++++++++-- >> libstdc++-v3/src/c++23/std.cc.in | 2 +- >> .../23_containers/mdspan/layout_traits.h | 4 + >> .../mdspan/submdspan/submdspan.cc | 377 +++++++++ >> .../mdspan/submdspan/submdspan_mapping.cc | 301 ++++++++ >> .../mdspan/submdspan/submdspan_neg.cc | 179 +++++ >> 6 files changed, 1514 insertions(+), 62 deletions(-) >> create mode 100644 >> libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan.cc >> create mode 100644 >> libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan_mapping.cc >> create mode 100644 >> libstdc++-v3/testsuite/23_containers/mdspan/submdspan/submdspan_neg.cc >> >> -- >> 2.52.0 >> >>
