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

P2599 mdspan::size_type should be index_type #1255

Closed
brycelelbach opened this issue Jun 19, 2022 · 7 comments
Closed

P2599 mdspan::size_type should be index_type #1255

brycelelbach opened this issue Jun 19, 2022 · 7 comments
Labels
C++23 Targeted at C++23 IS Ship vehicle: IS LWG Library mdspan plenary-approved Papers approved for inclusion in their target vehicle by plenary vote. tentatively-ready-for-plenary Reviewed between meetings; ready for a vote.
Milestone

Comments

@brycelelbach
Copy link

P2599R0 mdspan::size_type should be index_type (Nevin Liber)

@brycelelbach brycelelbach added LEWG Library Evolution C++23 Targeted at C++23 IS Ship vehicle: IS B2 - improvement Bucket 2 as described by P0592: bug fixes, performance improvements, integration fixes for/between e ready-for-library-evolution-meeting-review This paper needs to be discussed at a Library Evolution meeting size - small paper size estimate scheduled-for-library-evolution This paper has been scheduled for one of the groups: LEWG, LEWG Incubator, or a Mailing List review labels Jun 19, 2022
@brycelelbach
Copy link
Author

brycelelbach commented Jun 19, 2022

2022-06-07 Library Evolution Telecon

P2599R0: mdspan::size_type should be index_type

2022-06-07 Library Evolution Telecon Minutes

Chair: Bryce Adelstein Lelbach

Minute Taker: Ben Craig

Champion: Nevin Liber

Start: 2022-06-07 12:03 Eastern

POLL: Send P2599R0 (mdspan::size_type should be index_type) to Library for C++23 classified as an improvement (B2), to be confirmed with a Library Evolution electronic poll.

Strongly Favor Weakly Favor Neutral Weakly Against Strongly Against
2 7 2 2 1

Attendance: 21

# of Authors: 1

Author Position: SF

Outcome: No consensus.

SA: It's already a conscious choice by the user to use a signed type. So I don't think it will be surprising. The consistency of having it be called size_type is more important.

POLL: mdspan, extents, and layouts should have both an index_type (which is whatever the user provides for the first template parameter to extents) and a size_type (which is make_unsigned_t<index_type>).

Strongly Favor Weakly Favor Neutral Weakly Against Strongly Against
3 9 1 1 0

Attendance: 19

# of Authors: 1

Author Position: SF

Outcome: Strong consensus in favor, and stronger consensus than the paper as written.

WA: It's additional complexity.

End: 12:13

Summary

This paper proposes that we should rename mdspan's size_type to index_type, now that the index type is parameterized and may be either unsigned or signed.
Some felt that it might be surprising to users for mdspan to not use the familiar size_type name.
Others felt that it would be surprising for size_type to be an alias for a signed type, which is the status quo.
We decided that mdspan should have an index_type, which is the index type provided by the user, and also a size_type, which is make_unsigned_t<index_type>.

Next Steps

Produce and publish P2559R1 (mdspan::size_type should be index_type), making index_type an alias for the user-provided index type and size_type an alias for make_unsigned_t<index_type>.

@brycelelbach
Copy link
Author

2022-06-21 Library Evolution Telecon

P2599R1: mdspan::size_type should be index_type

2022-06-21 Library Evolution Telecon Minutes

Chair: Bryce Adelstein Lelbach

Minute Taker: Ben Craig

Champion: Nevin Liber

Start: 2022-06-21 11:15 Eastern

Does this paper have:

  • Changes Library Evolution previously requested?
    • Yes

Give the paper a clearer name now that the design intent has changed to prevent any confusion.

What should mdspan::size return?

  • P0009R16: size_type.
  • P0009R17 (after we parameterized index type): size_t.
  • P2599R1: index_type.

mdspan::size should probably return size_type.

What about layout mappings and mdpan's required_span_size?
This should remain index_type for performance.

What about layout mappings operator()?
The result of that is fed into accessors, which take a size_t.

Why do accessors take size_t instead of index_type?

Why doesn't layout mapping use operator[] instead of operator()?
Because it's a function object.

POLL: Modify P2599R1 (mdspan::size_type should be index_type) such that mdspan::sizes return type is size_type, and send the modified paper to Library for C++23 classified as B2 - Improvement, to be confirmed with a Library Evolution electronic poll.

Strongly Favor Weakly Favor Neutral Weakly Against Strongly Against
5 8 0 1 0

Attendance: 29

# of Authors: 1

Author Position: SF

Outcome: Strong consensus in favor.

WA: This is a late change.

End: 11:41

Summary

This paper proposes that mdspan should have an index_type, which is the index type provided by the user, and also a size_type, which is make_unsigned_t<index_type>.

We discussed what mdspan::size should return.
Before mdspan was parameterized on index type, it returned size_type.
After that change, it returned size_t.

The paper proposed that mdspan::size should be changed to return index_type.
We discussed this and believe that the new size_type is a more suitable return type.
We also discussed whether required_span_size on layouts should return index_type or
size_type, and decided that it should return index_type for performance.
We also considered what type accessors should take and return.
Currently, accessors are not parameterized on the index type, and expect and return size_t.

We forwarded the paper, with the modification of mdspan::size returning size_type, to Library for C++23, pending confirmation with an electronic poll.

Next Steps

  • Produce and publish P2599R2 (mdspan::size_type should be index_type), improving the paper title and changing mdspan::size's return type to size_type.
  • Take an electronic poll to confirm sending P2599R2 (mdspan::size_type should be index_type) to Library for C++23 classified as B2 - Improvement.

@brycelelbach brycelelbach added ready-for-library-evolution-electronic-poll This paper needs to undergo a Library Evolution electronic poll needs-revision Paper needs changes before it can proceed and removed ready-for-library-evolution-meeting-review This paper needs to be discussed at a Library Evolution meeting scheduled-for-library-evolution This paper has been scheduled for one of the groups: LEWG, LEWG Incubator, or a Mailing List review labels Jun 21, 2022
@wg21bot
Copy link
Collaborator

wg21bot commented Jun 24, 2022

P2599R1 mdspan::size_type should be index_type (Nevin Liber)

@wg21bot wg21bot removed the needs-revision Paper needs changes before it can proceed label Jun 24, 2022
@wg21bot wg21bot added this to the 2022-telecon milestone Jun 24, 2022
@wg21bot
Copy link
Collaborator

wg21bot commented Jun 24, 2022

P2599R2 index _type & size_type in mdspan (Nevin Liber)

@brycelelbach brycelelbach added the scheduled-for-library-evolution This paper has been scheduled for one of the groups: LEWG, LEWG Incubator, or a Mailing List review label Jun 29, 2022
@brycelelbach brycelelbach added LWG Library lwg-pending LWG Chair needs to disposition labels Jul 8, 2022
@JeffGarland
Copy link
Member

LWG reviewed and approved 2022-06-24 pending LEWG vote
Notes: https://wiki.edg.com/bin/view/Wg21telecons2022/P2599-20220624

poll: Put P2599R2 into C++23 pending LEWG approval

F A N
18 0 0

@JeffGarland JeffGarland added tentatively-ready-for-plenary Reviewed between meetings; ready for a vote. and removed lwg-pending LWG Chair needs to disposition B2 - improvement Bucket 2 as described by P0592: bug fixes, performance improvements, integration fixes for/between e size - small paper size estimate labels Jul 8, 2022
@brycelelbach
Copy link
Author

2022-07 Library Evolution Electronic Poll Outcomes

Poll 1.12: Send [P2599R2] index_type & size_type In mdspan to Library Working Group for C++23, classified as an improvement of an existing feature ([P0592R4] bucket 2 item).

Strongly Favor Weakly Favor Neutral Weakly Against Strongly Against
14 7 2 1 0

Outcome: Consensus in favor.

@brycelelbach brycelelbach removed LEWG Library Evolution ready-for-library-evolution-electronic-poll This paper needs to undergo a Library Evolution electronic poll scheduled-for-library-evolution This paper has been scheduled for one of the groups: LEWG, LEWG Incubator, or a Mailing List review labels Jul 22, 2022
@cor3ntin cor3ntin added the plenary-approved Papers approved for inclusion in their target vehicle by plenary vote. label Jul 25, 2022
@tkoeppe
Copy link

tkoeppe commented Aug 11, 2022

This has been applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++23 Targeted at C++23 IS Ship vehicle: IS LWG Library mdspan plenary-approved Papers approved for inclusion in their target vehicle by plenary vote. tentatively-ready-for-plenary Reviewed between meetings; ready for a vote.
Projects
None yet
Development

No branches or pull requests

5 participants