Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<mdspan>: Product code review #3971

Merged

Conversation

StephanTLavavej
Copy link
Member

  • Unconditionally strengthen mdspan::is_always_MEOW().
    • They return mapping_type::is_always_MEOW() which is required to be a constant expression of type bool (N4950 [mdspan.mdspan.overview]/3, [mdspan.layout.policy.reqmts]/1, [mdspan.layout.reqmts]/22,24,26). The Standard should simply be enhanced to say noexcept here.
    • Also introduce constexpr bool _Result to make this extra clear and improve debug codegen.
  • Drop strengthened comment; _Access_impl() is _Ugly.
  • Follow N4950 [mdspan.mdspan.cons]/8.1 by saying const _OtherIndexType& in this constraint.
  • In default_accessor, define access() before offset() to follow the order of N4950 [mdspan.accessor.default.overview].
  • layout_stride::template mapping is unnecessary; layout_stride is not a template.
  • Add explicit to the deduction guide extents(_Integrals...), depicted by N4950 [mdspan.extents.overview] and [mdspan.extents.cons]/12.
  • Style: Reorder the template parameters for _Contains_multidimensional_index() to match its function parameters; it's always called with template argument deduction.
  • Strengthen operator[] for span/array. Fine-grained commits for clarity:
    • Step 1: Move _Multidimensional_access() up.
    • Step 2: Add conditional noexcept to _Multidimensional_access(), not commented as strengthened because it's _Ugly.
      • It's now identical to the real multidimensional subscript operator except for the name, the comment, and the intentional lack of constraints.
    • Step 3: Add as_const() to the array overload, so it exactly matches the span overload.
      • This is what N4950 [mdspan.mdspan.members]/6 depicts. It was being implicitly skipped before (const array& has the same effect), but adding this will make the following unification clearer.
    • Step 4: Replace lambdas with _Multidimensional_subscript().
      • This unifies them by wrapping the array in a span.
    • Step 5: Add conditional noexcept to _Multidimensional_subscript (not commented, it's _Ugly), then strengthen operator[] for span/array.
      • This brings the span/array subscript operators up to parity with the multidimensional subscript operator.

They return `mapping_type::is_always_MEOW()` which is required to be a constant expression of type `bool` (N4950 \[mdspan.mdspan.overview\]/3, \[mdspan.layout.policy.reqmts\]/1, \[mdspan.layout.reqmts\]/22,24,26). The Standard should simply be enhanced to say `noexcept` here.

Also introduce `constexpr bool _Result` to make this extra clear and improve debug codegen.
…the order of N4950 \[mdspan.accessor.default.overview\].
…ted by N4950 \[mdspan.extents.overview\] and \[mdspan.extents.cons\]/12.
…l_index()` to match its function parameters; it's always called with template argument deduction.
…ot commented as strengthened because it's `_Ugly`.

It's now identical to the real multidimensional subscript operator except for the name, the comment, and the intentional lack of constraints.
…es the `span` overload.

This is what N4950 \[mdspan.mdspan.members\]/6 depicts. It was being implicitly skipped before (`const array&` has the same effect), but adding this will make the following unification clearer.
This unifies them by wrapping the `array` in a `span`.
…not commented, it's `_Ugly`), then strengthen `operator[]` for `span`/`array`.

This brings the `span`/`array` subscript operators up to parity with the multidimensional subscript operator.
@StephanTLavavej StephanTLavavej added the mdspan C++23 mdspan label Aug 15, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner August 15, 2023 23:25
@StephanTLavavej StephanTLavavej merged commit b60ac58 into microsoft:feature/mdspan2 Aug 15, 2023
@StephanTLavavej StephanTLavavej deleted the final-mdspan branch August 15, 2023 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mdspan C++23 mdspan
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant