On 7/3/25 12:45, Jonathan Wakely wrote:
On Thu, 3 Jul 2025 at 11:12, Tomasz Kaminski <tkami...@redhat.com> wrote:
On Thu, Jul 3, 2025 at 12:08 PM Luc Grosheintz <luc.groshei...@gmail.com> wrote:
The reasoning for this approach was:
1. The mapping::operator() and mdspan::operator[] have the same
precondition; and mdspan::operator[] calls mapping::operator().
Yes, although a user-defined mapping might not bother to check
preconditions. So in order for us to implement the required check in
operator[] we really need to check it there.
We could check it in *both* places, and assume that the compiler will
see that the second check is entirely redundant.
We could also check in mapping::operator() and then in
mdspan::operator[] do something like:
if constexpr (!__is_std_mapping<mapping_type>)
__glibcxx_assert(...);
So if we know there's a check in mapping::operator() then don't bother
to check in operator[] as well.
This is an excellent description of how it's currently implemented. We
only skip the check in mdspan::operator[] if we know that the equivalent
is performed in mapping::operator().
2. The place I chose to check the precondition is where we already
have both the index and the extent in L1 and almost certainly in a
register. The hope was that together with branch prediction, this
will be a reasonably cheap place to put the check.
3. The layouts are highly valuable on their own. I've implemented
that piece of logic numerous times in different contexts; and it's
wonderful that soon we can convert `i, j, k` to a linear index easily
using the standard library.
Therefore, I didn't want to skip them in mapping::operator() because
they're a guard against out of bounds accesses, e.g. in a user-defined
dynamically allocated, owning, multi-dimensional array.
I think such types would have their own bounds checks, contracts, preconditions.
Not if it's just something wrapping a unique_ptr<T[]>, for example.
Very valid points. I'd like to add that out of bounds accesses when
iterating over multi-dimensional arrays is a reasonably frequent bug
while developing scientific codes, e.g. write (i, i) instead of (i, j)
or get an N and M mixed up. If unchecked it manifests itself in cryptic
ways; if checked it's usually trivial to fix.
To me checking in mapping::operator() seems like a good idea, I'm sure
it would have caught bugs in code I wrote.
There's a few paths forwards:
1. Remove the check from mapping::operator() and unconditionally
check in mdspan::operator[].
I would go for option 1.
My original preference was option 2, but I've convinced myself that we
need the checks in operator[]. Rather than repeat them in both places,
I think I'm OK with option 1 too.
I'll downgrade them to _GLIBCXX_ASSERT_DEBUG and make the check in
mdspan::operator[] unconditional.
2. Leave it as is and return when we do optimization or hardening.
3. Start measuring to figure out the cost of these checks; and then
decide.
I'm open to all three.
