mhoemmen commented 6 months ago

P2642: LWG 2024/02/14 review

[x] Paragraph numbers are off at the top of the wording. Somehow they are mixing up edit instructions and text. (The paragraph numbers are the ones in the diff. They need to be right -- what do we do?) It's not clear enough just indenting the block quote a bit -- perhaps change the background to make the block quotes differ more. Also, for the code additions, make them green and include more diff context.
[x] For least multiple at least: 2.4. It's math, not code; what font should we use? It can't be code font, because we use it in "is representable as" expressions. Change it to imitate this model: https://eel.is/c++draft/mdspan.layout.stride#expo-1 (all caps, italics, kabob case). (Fixed in https://github.com/ORNL/cpp-proposals-pub/pull/449 .)

[mdspan.layout.leftpadded]

[x] 2-parameter constructor, taking extents and run-time padding value: precondition was changed. We'll need both the "product of" precondition and the original one. Add the original one back in.

[mdspan.layout.leftpadded.obs]

`operator()`

[x] Missing underscore-v on 3.3 is_nothrow_constructible (Fixed in PR #449 ).
[x] Para 5: lose the semicolon, leave the period (Fixed in PR #449 ).
[x] Para 6.1 and 7.1 say the same thing differently. Prefer the 6.1 form, "equals zero or one." (Fixed in PR #449 .)
[x] Para 6.2: "if neither ... equals dynamic_extent, ...." (Fixed in PR #449 .)
[x] Para 9.1 (is_exhaustive)): Novel if-then-else logic. "otherwise" is dangling, unlike para 6.3. Make it more like paras 6 and 7.
[x] Para 10.2, operator==: omit typename on the left-hand side of the equality comparison.
[x] Param 11: Fix the "if" (should be "if and only if"), and explain what happens if they aren't true. Should be like https://eel.is/c++draft/views.multidim#mdspan.layout.stride.obs-8 , that is, like layout_stride::mapping::operator==.

[mdspan.layout.rightpadded]

Para 5: Mandates on a type is novel, if arguably better. Normally we say "is ill-formed." But it's not novel because it's what we do for mdspan. However, we only define what it means for Mandates to appear on a function, not on a type. We should fix [structure.specifications] then, not this. (No need to change this proposal.)

[x] Class itself, Para 5.3: fix the Mandates in the same way as the Preconditions: product of... (that and the other thing).
[x] Paragraph numbers recent to 4 after Para 5.3 above. Fix that (fixed in PR #449 ).
[x] Move the definition of static-padding-stride so it's not part of the for-the-whole-class bit. Add a new section for those two exposition-only members.

Why is static-padding-stride zero in the rank 0 or 1 case? It would be more consistent with the definition for it to be 1. (This is OK; no need to change the proposal.)

[mdspan.layout.rightpadded.cons]

[x] constexpr mapping() noexcept could be defined inline as delegating.
[ ] Para 2.2, we could drop "and padding_value does not equal dynamic_extent", and use the fact that static-padding-stride is zero along with the definition of LEAST-MULTIPLE-AT-LEAST. (This didn't make sense to me. I'm leaving it for now.)
[x] Para 5, Preconditions: treat each one as a separate sentence, each ending with a period. (However, Effects are a single sentence, because they are not independent.)
[x] Para 6, Effects: Make this a single sentence with no bullets and no semicolon.

(Converting constructor from layout_right::mapping)

[x] Para 8: Double if (remove the "if" on the first bullet). Change as follows.

Mandates:
  If OtherExtents::rank() > 1, then
  (static-padding-stride == dynamic_extent)
 || (OtherExtents::static_extents(rank_ - 1) == dynamic_extent) || (static-padding-stride == OtherExtents::static_extents(rank_ - 1)) is true

[x] Remove the [basic.fundamental] cross reference from Para 9.2 (Preconditions).
[x] Para 10: Period at the end, not a semicolon.

Finishing with converting constructor from layout_right::mapping, right before converting constructor from layout_stride::mapping.

mhoemmen commented 6 months ago

Preparation for 2024/02/27 LWG review:

"Paragraph numbers are off at the top of the wording." (This is still the case; it awaits more time for formatting.)
"Para 2.2, we could drop 'and padding_value does not equal dynamic_extent'" (I didn't see the benefit, so I left this as-is. The review didn't call this incorrect. I would consider changing this with an example of how to word it.)
I also improved consistency between wording of layout_left_padded::mapping and layout_left_padded::mapping.

crtrott commented 6 months ago

I can still fix the numbers was working on the stuff you hadn't fixed yet this afternoon, but now had noticed you updated more ...

mhoemmen commented 6 months ago

(We chatted offline just now.)

mhoemmen commented 6 months ago

"Paragraph numbers are off at the top of the wording." (This is still the case; it awaits more time for formatting.)

Fixed last night. LWG is pleased on 2024/02/28.

mhoemmen commented 6 months ago

@crtrott Regarding the following point:

Para 2.2, we could drop "and padding_value does not equal dynamic_extent", and use the fact that static-padding-stride is zero along with the definition of LEAST-MULTIPLE-AT-LEAST. (This didn't make sense to me. I'm leaving it for now.)

This didn't sound like a wording correctness issue. We can drop this unless it comes up again.

ORNL / cpp-proposals-pub