kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
401 stars 66 forks source link

Access with bounds checking using `::at` #300

Open stephanlachnit opened 9 months ago

stephanlachnit commented 9 months ago

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 commented 9 months ago

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

crtrott commented 9 months ago

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 commented 9 months ago

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 https://github.com/cplusplus/papers/issues/1501#issuecomment-1806852050 yes.

stephanlachnit commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

@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 commented 6 months ago

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 commented 5 months ago

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.