Closed beinhaerter closed 1 year ago
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!
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.
Is the static assert therefore effectively checking that
N < std::numeric_limits<std::ptrdiff_t>::max()
, which therefore means thati < 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.
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?
@beinhaerter indeed it looks like N <= std::numeric_limits<std::size_t>::max() / 2
would be correct.
I was just looking at the
gsl::at
overloads and don't understand theExpects
check. For example for the C style array version.Checking that
i
is non-negative makes sense. But why is theni
checked againstnarrow_cast<index>(N)
? For example on a 32 bit platform when having an array with 0xff'ff'ff'ff elements and when accessing elementi==0
why would I want to check thati<narrow_cast<index>(0xff'ff'ff'ff)
which isi<-1
? Wouldn't it make more sense to checknarrow_cast<std::size_t>(i) < N
? The same question applies to the other overloads ofgsl::at
.