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
>>

Reply via email to