[PATCH] libstdc++: Improve diagnostics for ill-formed std::_Destroy and std::_Destroy_n [PR120390]

[Jonathan Wakely]

On Wed, 11 Jun 2025 at 10:24, Tomasz Kaminski <tkaminsk@redhat.com> wrote:
>
>
>
> On Tue, Jun 10, 2025 at 12:37 PM Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>> By using std::is_trivially_destructible instead of the old
>> __has_trivial_destructor built-in we no longer need the static_assert to
>> deal with types with deleted destructors. All non-destructible types,
>> including those with deleted destructors, will now give user-friendly
>> diagnostics that clearly explain the problem.
>>
>> Also combine the _Destroy_aux and _Destroy_n_aux class templates used
>> for C++98 into one, so that we perform fewer expensive class template
>> instantiations.
>>
>> libstdc++-v3/ChangeLog:
>>
>>         PR libstdc++/120390
>>         * include/bits/stl_construct.h (_Destroy_aux::__destroy_n): New
>>         static member function.
>>         (_Destroy_aux<true>::__destroy_n): Likewise.
>>         (_Destroy_n_aux): Remove.
>>         (_Destroy(ForwardIterator, ForwardIterator)): Remove
>>         static_assert. Use is_trivially_destructible instead of
>>         __has_trivial_destructor.
>>         (_Destroy_n): Likewise. Use _Destroy_aux::__destroy_n instead of
>>         _Destroy_n_aux::__destroy_n.
>>         * testsuite/20_util/specialized_algorithms/memory_management_tools/destroy_neg.cc:
>>         Adjust dg-error strings. Move destroy_n tests to ...
>>         * testsuite/20_util/specialized_algorithms/memory_management_tools/destroy_n_neg.cc:
>>         New test.
>>         * testsuite/23_containers/vector/cons/destructible_debug_neg.cc:
>>         Adjust dg-error strings.
>>         * testsuite/23_containers/vector/cons/destructible_neg.cc:
>>         Likewise.
>> ---
>>
>> Tested x86_64-linux.
>
> LGTM.
> Not directly related to the patch, but I have noticed that for c++98, for trivially destructible
> types we make sure that we traverse range (we call advance) however in C++11+ mode
> we do not do that. Something changed in standard?

I think they should be equivalent. For C++98 the
_Destroy_aux<true>::__destroy_n function calls advance and returns the
result:


>> @@ -189,6 +198,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>        template<typename _ForwardIterator>
>>          static void
>>          __destroy(_ForwardIterator, _ForwardIterator) { }
>> +
>> +      template<typename _ForwardIterator, typename _Size>
>> +       static _ForwardIterator
>> +       __destroy_n(_ForwardIterator __first, _Size __count)
>> +       {
>> +         std::advance(__first, __count);
>> +         return __first;
>> +       }
>>      };
>>  #endif

For C++11 we do the same directly in the _Destroy_n function:

>> @@ -260,10 +247,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>        typedef typename iterator_traits<_ForwardIterator>::value_type
>>                         _Value_type;
>>  #if __cplusplus >= 201103L
>> -      // A deleted destructor is trivial, this ensures we reject such types:
>> -      static_assert(is_destructible<_Value_type>::value,
>> -                   "value type is destructible");
>> -      if constexpr (!__has_trivial_destructor(_Value_type))
>> +      if constexpr (!is_trivially_destructible<_Value_type>::value)
>>         for (; __count > 0; (void)++__first, --__count)
>>           std::_Destroy(std::__addressof(*__first));
>>  #if __cpp_constexpr_dynamic_alloc // >= C++20
>> @@ -275,7 +259,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>         std::advance(__first, __count);
>>        return __first;

^ here.