Add element access via at() to std::mdspan

[stephanlachnit]

Closes #300

Possible implementation for ::at element access to mdspan with boundary checks. The boundary check is implemented with a for loop, as I did not see any other way to achieve this. The error message is inspired by the std::span::at error message from libstdc++ (gcc-mirror/gcc@1fa85dc).

Note that this does not implement ::at element access to mdarray.

[stigrs]

Given that SizeTypes can be signed, it should also check for indices[r] < 0.

[stephanlachnit]

Given that SizeTypes can be signed, it should also check for indices[r] < 0.

Thanks for this comment! Is this true for std::span as well? At least in libstdc++, they do not check this:

https://github.com/gcc-mirror/gcc/blob/fd7dabc116b9abc40ee6aa25bcc5d240b8cc516a/libstdc%2B%2B-v3/include/std/span#L290-L300

[nmm0]

In span, I believe it's always just std::size_t, but mdspan has the size type as a template parameter for the extent and can be signed.

[stephanlachnit]

In span, I believe it's always just std::size_t, but mdspan has the size type as a template parameter for the extent and can be signed.

Ah right, thanks for pointing this out!

Edit: updated the pull request

[mhoemmen]

SizeType doesn't need to be integral here or even less-than comparable with int. mdspan behaves as if all operations on indices happen after conversion to index_type. Thus, should we consider instead converting to index_type first, as we do below?

Suggested change

	// Check for negative indices
	if constexpr(std::is_signed_v<SizeType>) {
	if(index < 0) {
	return true;
	}
	}
	// Check for negative indices
	if constexpr (std::is_signed_v<index_type>) {
	if (static_cast<index_type>(index) < 0) {
	return true;
	}
	}

[stephanlachnit]

Seems reasonable

[stephanlachnit]

After thinking about it again, I'm not sure if this is correct: imagine I have unsigned short as index_type and an extent of size of 65535. Now, I want to access with a signed short of value -10. What I end up getting is the element at index 65526, which is not the expected result.

[stephanlachnit]

I ended up reverting this: if SizeType is not integral or less-than comparable with int, std::is_signed_v should be false anyway. The question is, what happens if SizeType can be negative, but is is not integral or less-than comparable with int, while index_type is not signed?

[mhoemmen]

After thinking about it again, I'm not sure if this is correct: imagine I have unsigned short as index_type and an extent of size of 65535. Now, I want to access with a signed short of value -10. What I end up getting is the element at index 65526, which is not the expected result.

Thanks for pointing this out! The underlying issue, as I understand it, is that index-cast returns its input (with the same type, not cast to index_type) if the input type is integral-not-bool.

[mdspan.mdspan.members] 2 - 3 together say that the precondition on the input of mdspan::operator[] applies to extents_type::index-cast(std::move(indices...)). For the specific case you mentioned of SizeType = signed short, [mdspan.extents.expo] 9 explains that index-cast would just return signed short; it does not convert to index_type. -10 cannot be part of a multidimensional index in extents(), so this violates a precondition.

[mhoemmen]

Btw, I was looking through extents.hpp and noticed check_all_indices. It looks like the layout mappings using this. I think it could be refactored a bit to serve your needs.

[stephanlachnit]

if constexpr doesn't exist pre C++17, should I add a macro here? Or just go with if and assume the compiler optimizes for this anyway?

[stephanlachnit]

I don't quite get the build failure on MSVC, would need some help there...

[nmm0]

I'm of the opinion that we shouldn't add to the reserved names (i.e. __ prefix) even though it's consistent with the rest of the class since we do plan on removing them.

[nmm0]

I don't quite get the build failure on MSVC, would need some help there...

What if you used the requires clause from operator()? I think that operator[] is not used in the MSVC build so we may not have updated it:

  MDSPAN_TEMPLATE_REQUIRES(
    class... SizeTypes,
    /* requires */ (
      extents_type::rank() == sizeof...(SizeTypes) &&
      (detail::are_valid_indices<index_type, SizeTypes...>())
    )
  )