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

Access with bounds checking using ::at #300

Open
stephanlachnit opened this issue Nov 27, 2023 · 8 comments · May be fixed by #302
Open

Access with bounds checking using ::at #300

stephanlachnit opened this issue Nov 27, 2023 · 8 comments · May be fixed by #302

Comments

@stephanlachnit
Copy link
Contributor

stephanlachnit commented Nov 27, 2023

In C++26, std::span will get a ::at member function. It's probably a good idea to do this for mdspan as well. The function signatures would be the same as in operator[]:

template< class... OtherIndexTypes >
constexpr reference at( OtherIndexTypes... indices ) const;
template< class OtherIndexType >
constexpr reference at( std::span<OtherIndexType, rank()> indices ) const;
template< class OtherIndexType >
constexpr reference at( const std::array<OtherIndexType, rank()>& indices ) const;

Another neat advantage is that this does not require the multidimensional subscript operator, meaning that the signature is identical between in pre C++23 code.

Related discussion on std-proposals: https://lists.isocpp.org/std-proposals/2023/11/index.php#msg8301

@mhoemmen
Copy link
Contributor

@stephanlachnit Thanks for the suggestion! : - ) Are you considering writing that paper?

@crtrott
Copy link
Member

crtrott commented Nov 27, 2023

I think this is fine. There aren't really any additional concerns beyond the ones which would show up in span. So if span gets it mdspan also should. Was the proposal for span::at voted into C++26 in Kona? I didn't check.

@stephanlachnit
Copy link
Contributor Author

stephanlachnit commented Nov 27, 2023

Thanks for the suggestion! : - ) Are you considering writing that paper?

I have never written a C++ proposal and unfortunately would not be able to present in person at an ISO C++ standards meeting. I can try to write a paper draft, but I would probably need some help.

Edit: I started drafting a proposal at https://github.com/stephanlachnit/mdspan-at

I think this is fine. There aren't really any additional concerns beyond the ones which would show up in span. So if span gets it mdspan also should. Was the proposal for span::at voted into C++26 in Kona? I didn't check.

I'm not sure where to check, but according to cplusplus/papers#1501 (comment) yes.

@stephanlachnit stephanlachnit linked a pull request Nov 27, 2023 that will close this issue
@stephanlachnit
Copy link
Contributor Author

@mhoemmen @crtrott I decided to give the paper a shot anyway, please take a look at my draft with wording relative to N4964 at https://stephanlachnit.github.io/mdspan-at/. Would you like to be co-authors and present it to the C++ standards committee?

@dalg24
Copy link
Member

dalg24 commented Nov 29, 2023

I looked at the wording and I was wondering whether

Effects: Equivalent to: operator[](OtherIndexTypes... indices) with bounds checking

is sufficient and you don't need to repeat the constraints.
Christian and Mark spent quite a bit of time in LWG so they would know.

@mhoemmen
Copy link
Contributor

@stephanlachnit wrote:

Would you like to be co-authors and present it to the C++ standards committee?

Thank you for taking interest in this. I should have expressed myself better: My personal view is that I'd rather not spend time adding .at to mdspan. I'm not speaking for Christian or any other people, just for myself. I wouldn't object if someone added it to mdspan, but I personally don't want to spend time on it.

@stigrs
Copy link

stigrs commented Mar 17, 2024

Given that OtherIndexTypes can be signed, shouldn't it also throw out_of_range if indices[i] < 0 for any indices[i] in indices? Or do you allow negative indexing?

@mhoemmen
Copy link
Contributor

My personal view is that I'd rather not spend time adding .at to mdspan.

Note that a custom layout could do multidimensional bounds checking, and a custom accessor could do (1-D) bounds checking for any layout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants