On Tue, 1 Jul 2025 at 11:32, Tomasz Kaminski <tkami...@redhat.com> wrote: > > Hi, > More of the review will be later, but I have noticed that you have added > preconditions checks > to the layouts, and then avoid checking them inside the operator[] of the > mdspan. This is general > sounds good. > > However, the precondition on mdspan::operator[] is now hardened: > https://eel.is/c++draft/views.multidim#mdspan.mdspan.members-3. > This implies that breaking this precondition is a contract violation, and > thus the user may expect a contract violation handler to be invoked > for it. Amongst the information provided to the handler via > contract_violation object (https://eel.is/c++draft/support.contract.violation) > is source_location, that includes the name.
Even without contracts, our __glibcxx_assert macro includes the __PRETTY_FUNCTION__ string in the abort message. > Given that I think we want to always check the extents in operator[] of > mdspan, and thus remove > the checks from layout (to avoid duplication). I think it's probably QoI whether the contract checks happen directly in the operator[] function or in something that it calls. If doing the checks in operator[] has no extra runtime cost (e.g. due to them getting duplicated), then I think it is better to do them there, so that errors are reported from the "correct" place. But if it hurts performance, I don't think it's essential to check them there. > > Hope, the above makes sense. > > Regards, > Tomasz > > On Fri, Jun 27, 2025 at 11:12 AM Luc Grosheintz <luc.groshei...@gmail.com> > wrote: >> >> Previously the prerequisite that the arguments passed to operator() are >> a multi-dimensional index (of extents()) was not checked. >> >> This commit adds the __glibcxx_asserts and the required tests. >> >> libstdc++-v3/ChangeLog: >> >> * include/std/mdspan: Check prerequisites of >> layout_*::operator(). >> * testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc: >> Add tests for prerequisites. >> >> Signed-off-by: Luc Grosheintz <luc.groshei...@gmail.com> >> --- >> libstdc++-v3/include/std/mdspan | 4 +++ >> .../mdspan/layouts/class_mandate_neg.cc | 26 +++++++++++++++++++ >> 2 files changed, 30 insertions(+) >> >> diff --git a/libstdc++-v3/include/std/mdspan >> b/libstdc++-v3/include/std/mdspan >> index c72a64094b7..39d02ac08df 100644 >> --- a/libstdc++-v3/include/std/mdspan >> +++ b/libstdc++-v3/include/std/mdspan >> @@ -441,6 +441,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> _IndexType __mult = 1; >> auto __update = [&, __pos = 0u](_IndexType __idx) mutable >> { >> + __glibcxx_assert(cmp_less(__idx, __exts.extent(__pos))); >> __res += __idx * __mult; >> __mult *= __exts.extent(__pos); >> ++__pos; >> @@ -651,6 +652,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> auto __update = [&, __pos = __exts.rank()](_IndexType) mutable >> { >> --__pos; >> + __glibcxx_assert(cmp_less(__ind_arr[__pos], >> + __exts.extent(__pos))); >> __res += __ind_arr[__pos] * __mult; >> __mult *= __exts.extent(__pos); >> }; >> @@ -822,6 +825,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> { >> auto __update = [&, __pos = 0u](_IndexType __idx) mutable >> { >> + __glibcxx_assert(cmp_less(__idx, >> __m.extents().extent(__pos))); >> __res += __idx * __m.stride(__pos++); >> }; >> (__update(__indices), ...); >> diff --git >> a/libstdc++-v3/testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc >> b/libstdc++-v3/testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc >> index 7091153daba..f5aab22aff0 100644 >> --- >> a/libstdc++-v3/testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc >> +++ >> b/libstdc++-v3/testsuite/23_containers/mdspan/layouts/class_mandate_neg.cc >> @@ -45,4 +45,30 @@ auto b5 = B<5, std::layout_stride, std::layout_right>(); >> // { dg-error "require >> auto b6 = B<6, std::layout_stride, std::layout_left>(); // { dg-error >> "required from" } >> auto b7 = B<7, std::layout_stride, std::layout_stride>(); // { dg-error >> "required from" } >> >> +template<typename Layout> >> + constexpr bool >> + test_linear_index_0d() >> + { >> + auto m = typename Layout::mapping<std::extents<int, 0>>{}; >> + (void) m(0); // { dg-error "expansion of" } >> + return true; >> + } >> +static_assert(test_linear_index_0d<std::layout_left>()); // { dg-error >> "expansion of" } >> +static_assert(test_linear_index_0d<std::layout_right>()); // { dg-error >> "expansion of" } >> +static_assert(test_linear_index_0d<std::layout_stride>()); // { dg-error >> "expansion of" } >> + >> +template<typename Layout> >> + constexpr bool >> + test_linear_index_3d() >> + { >> + auto m = typename Layout::mapping<std::extents<int, 3, 5, 7>>{}; >> + (void) m(2, 5, 5); // { dg-error "expansion of" } >> + return true; >> + } >> +static_assert(test_linear_index_3d<std::layout_left>()); // { dg-error >> "expansion of" } >> +static_assert(test_linear_index_3d<std::layout_right>()); // { dg-error >> "expansion of" } >> +static_assert(test_linear_index_3d<std::layout_stride>()); // { dg-error >> "expansion of" } >> + >> // { dg-prune-output "must be representable as index_type" } >> +// { dg-prune-output "non-constant condition for static assertion" } >> +// { dg-prune-output "__glibcxx_assert" } >> -- >> 2.49.0 >>