Optional extent checking on array dimensions

[M-Kusumgar]

Thank you for developing an awesome library! It would be useful to be able to toggle extent checking at runtime for testing. We have encountered a couple of rare problems with indices going past their extents but still mapping to a valid flat offset, such as a 2 x 3 x 4 tensor with index (3, 1, 1) in column major ordering that returns a flat offset of

3 + 1 * (2) + 1 * (2 * 3) = 11 (less than 2 * 3 * 4 = 24 so valid)

seems like it would be fairly easy to just add something like this below line 481

#ifdef NDA_BOUND_CHECKING
if (at - min_ >= extent_) {
  throw std::runtime_error(...)
}
#endif

to catch these greater than extent errors. Do you think this is reasonable?

[dsharlet]

Thanks for the suggestion. A while back, we had an at() method, which like std::vector::at, did bounds checks (or at least we had a TODO to do that), but I ended up simply removing at: 0babbd3)

I think std::vector's behavior in this way is weird (no bounds check in operator[], bounds check in at()), but I was thinking of imitating it.

It would need to be as you suggest, behind an opt-in build flag. It's possible that existing code uses operator() in a way that computes an address of an out of bounds value, but doesn't dereference it, and just uses it for pointer arithmetic.

(Now I wonder... maybe this is why std::vector is the way it is... someone wanted to add bounds checking, but adding it to an existing API is dangerous in this way. Adding a new API solves that problem.)

[M-Kusumgar]

That makes sense, a different API would definitely be the more sensible route, was there a reason why you removed the at() method/would you be opposed to adding that back in, in a way that it does bounds checking? I am happy to try and raise a PR if this is a feature you would want back in?

Also potentially another thought is instead of polluting the flat offset method in dim, we can instead switch (via enable_ifs/ifdefs) on the indexing operator in the actual array class to use the at() method, that would also keep the blast radius minimal, and would also have the consequence of keeping existing code the same but allowing people to bounds check optionally in only array accesses