Wrong Expects in gsl::at?

[beinhaerter]

I was just looking at the gsl::at overloads and don't understand the Expects check. For example for the C style array version.

template <class T, std::size_t N>
    constexpr T& at(T (&arr)[N], const index i)
{
    Expects(i >= 0 && i < narrow_cast<index>(N));
    return arr[narrow_cast<std::size_t>(i)];
}

Checking that i is non-negative makes sense. But why is then i checked against narrow_cast<index>(N)? For example on a 32 bit platform when having an array with 0xff'ff'ff'ff elements and when accessing element i==0 why would I want to check that i<narrow_cast<index>(0xff'ff'ff'ff) which is i<-1? Wouldn't it make more sense to check narrow_cast<std::size_t>(i) < N?
The same question applies to the other overloads of gsl::at.

[hsutter]

Off the top of my head while on vacation:

I thought that having more elements than half the number of addressable locations generally isn't legal or practical anyway (IIRC of iterator requirements, pointer/iterator subtraction, I've paged out the exact reasons). So how about instead adding static_assert(N < std::numeric_limits<std::size_t>::max()/2-1);?

Edited to add: That is, instead of changing the run-time checks, add a compile-time check to disallow the case that would have other legality problems anyway? Then the run-time checks should be fine I think. Please correct if I missed something!

[dmitrykobets-msft]

Hi @hsutter,

Is this your intended change?

static_assert(N < std::numeric_limits<std::size_t>::max()/2-1);
Expects(i >= 0 && i < narrow_cast<index>(N));
return arr[narrow_cast<std::size_t>(i)];

Is the static assert therefore effectively checking that N < std::numeric_limits<std::ptrdiff_t>::max(), which therefore means that i < narrow_cast<index>(N) will never wrap?
And since std::ptrdiff_t holds a smaller positive value than std::size_t, the cast in the container access return arr[narrow_cast<std::size_t>(i)]; is also fine?

If so, I wonder if it makes sense to simply take the suggested implementation of:

Expects(i >= 0 && narrow_cast<std::size_t>(i) < N);
return arr[narrow_cast<std::size_t>(i)];

Since this is more straightforward and does not impose a static assertion which can (probably?) never be violated anyways.

[hsutter]

Is the static assert therefore effectively checking that N < std::numeric_limits<std::ptrdiff_t>::max(), which therefore means that i < narrow_cast<index>(N) will never wrap?

That's the idea, and I would have suggested the above form but I habitually wanted to avoid writing a new mixed unsigned/signed comparison. I generally prefer static assertions where possible... shift-left. It shouldn't ever fire, but if it does it's valuable information.

But I think the suggested implementation could be fine too.

Let's see what @gdr-at-ms thinks ... I'm happy with whichever he prefers.

[beinhaerter]

My PR suggests:

static_assert(N <= std::numeric_limits<std::size_t>::max() / 2, "We only support arrays up to half the bytes of the address space.");
Expects(i >= 0 && narrow_cast<std::size_t>(i) < N);
return arr[narrow_cast<std::size_t>(i)];

The static_assert is Herbs idea and I like it. It fires early at compile time and explains what is going wrong.
Regarding the Expects I think the changed code is clearer because the index checked in the Expects is exactly the index used in the return expression. And it would allow accessing the first 2^31 elements of containers with more than 2^31 elements. If you don't agree, I can revert the Expects and just keep the static_assert and the other minor changes.

Regarding the static_assert I was just rechecking. std::numeric_limits<std::size_t>::max() should be 0xffffffff. std::numeric_limits<std::size_t>::max() / 2 should be 0x7fffffff, and that should be std::numeric_limits<std::ptrdiff_t>::max(). So I think static_assert(N <= std::numeric_limits<std::size_t>::max() / 2) should be correct?

[dmitrykobets-msft]

@beinhaerter indeed it looks like N <= std::numeric_limits<std::size_t>::max() / 2 would be correct.