On Fri, 11 Apr 2025 at 12:21, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Fri, Apr 11, 2025 at 12:52 PM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> On 11/04/25 09:49 +0200, Tomasz Kaminski wrote:
>> >On Thu, Apr 10, 2025 at 6:29 PM Jonathan Wakely <jwak...@redhat.com> wrote:
>> >
>> >> For C++11 and later we can remvoe four overloads of _S_copy_chars and
>> >> use constexpr-if in the generic _S_copy_chars. This simplifies overload
>> >> resolution for _S_copy_chars, and also means that we optimize for other
>> >> iterators such as std::vector<char>::iterator.
>> >>
>> >> We still need all the _S_copy_chars overloads to be part of the explicit
>> >> instantiation definition, so make them depend on the macro that is
>> >> defined by src/c++11/string-inst.cc for that purpose.
>> >>
>> >> libstdc++-v3/ChangeLog:
>> >>
>> >> * include/bits/basic_string.h (_S_copy_chars): Replace overloads
>> >> with constexpr-if and extend optimization to all contiguous
>> >> iterators.
>> >> * src/c++11/string-inst.cc: Extend comment.
>> >> ---
>> >>
>> >> Tested x86_64-linux.
>> >>
>> >LGTM with one clarification question.
>> >
>> >>
>> >> libstdc++-v3/include/bits/basic_string.h | 31 ++++++++++++++++++------
>> >> libstdc++-v3/src/c++11/string-inst.cc | 3 ++-
>> >> 2 files changed, 25 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/libstdc++-v3/include/bits/basic_string.h
>> >> b/libstdc++-v3/include/bits/basic_string.h
>> >> index 886e7e6b19e..067c7915c76 100644
>> >> --- a/libstdc++-v3/include/bits/basic_string.h
>> >> +++ b/libstdc++-v3/include/bits/basic_string.h
>> >> @@ -468,6 +468,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>> >> traits_type::assign(__d, __n, __c);
>> >> }
>> >>
>> >> +#pragma GCC diagnostic push
>> >> +#pragma GCC diagnostic ignored "-Wc++17-extensions"
>> >> // _S_copy_chars is a separate template to permit specialization
>> >> // to optimize for the common case of pointers as iterators.
>> >> template<class _Iterator>
>> >> @@ -475,31 +477,44 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>> >> static void
>> >> _S_copy_chars(_CharT* __p, _Iterator __k1, _Iterator __k2)
>> >> {
>> >> +#if __cplusplus >= 201103L
>> >> + using _IterBase = decltype(std::__niter_base(__k1));
>> >> + if constexpr (__or_<is_same<_IterBase, _CharT*>,
>> >> + is_same<_IterBase, const _CharT*>>::value)
>> >> + _S_copy(__p, std::__niter_base(__k1), __k2 - __k1);
>> >> +#if __cpp_lib_concepts
>> >> + else if constexpr (contiguous_iterator<_Iterator>
>> >> + && is_same_v<iter_value_t<_Iterator>,
>> >> _CharT>)
>> >> + {
>> >> + const auto __d = __k2 - __k1;
>> >> + (void) (__k1 + __d); // See P3349R1
>> >> + _S_copy(__p, std::to_address(__k1),
>> >> static_cast<size_type>(__d));
>> >> + }
>> >> +#endif
>> >> + else
>> >> +#endif
>> >> for (; __k1 != __k2; ++__k1, (void)++__p)
>> >> traits_type::assign(*__p, *__k1); // These types are off.
>> >> }
>> >> +#pragma GCC diagnostic pop
>> >>
>> >> - _GLIBCXX20_CONSTEXPR
>> >> +#if __cplusplus < 201103L || defined
>> >> _GLIBCXX_DEFINING_STRING_INSTANTIATIONS
>> >> static void
>> >> - _S_copy_chars(_CharT* __p, iterator __k1, iterator __k2)
>> >> _GLIBCXX_NOEXCEPT
>> >>
>> >The `_GLIBCXX_DEFINING_STRING_INSTANTIATIONS` seem to be used for
>> >c++11/string-inst.cc, in
>> >which case the noexcept was meaningful.
>>
>> Is it?
>
> I meant in a sense that `_GLIBCXX_NOEXCEPT` expanded to something. Nothing
> beyond that.
Ah OK :-)
Yes, it is a difference in the tokens compiled in string-inst.cc, but
I don't think it matters.
>>
>>
>> >The macro is not used in
>> >c++17/string-inst.cc file, so noexcept
>>
>> The src/c++17/string-inst.cc file only instantiates a handful of
>> member functions, and none of them uses _S_copy_chars. So I don't
>> think it matters for src/c++17/string-inst.cc at all.
>>
>> >would be part of the function signature, so I do not think we change
>> >emitted symbols. Is this the reason
>> >why did you remove it?
>>
>> I removed it because those overloads of _S_copy_chars are only visible
>> in the header for C++98, where _GLIBCXX_NOEXCEPT expands to nothing.
>>
>> You're correct that in src/c++11/string-inst.cc those overloads are
>> instantiated for C++11 (using -std=gnu++11), where it would expand to
>> 'noexcept'. But I don't think it's meaningful whether it's present or
>> not.
>>
>> In C++11 the noexcept isn't part of the type, and it never affects the
>> mangled name even in C++17. For these _S_copy_chars overloads, the
>> compiler can easily see that they don't throw, as they only call
>> std::char_traits<char>::assign or std::char_traits<char>::copy (or the
>> wchar_t equivalents) and those are noexcept. So for the explicit
>> instantiation definitions, the compiler knows they can't throw, and so
>> it never needs to use a noexcept landing pad to terminate if an
>> exception happens. So noexcept on the definition does nothing. Those
>> functions can't throw, and we don't need (or want) extra code emitted
>> to enforce the noexcept by terminating.
>>
>> So the noexcept is only meaningful at the call site, so that callers
>> can see that those functions don't throw (and don't need their own
>> exception handling code). The call sites in string-inst.cc should be
>> able to see that they don't throw (they're compiled with -O2, and I
>> checked on x86_64 and there are no calls to _S_copy_chars left in the
>> assembly code). The call sites in user code can only see those
>> overloads for C++98, where _GLIBCXX_NOEXCEPT expands to nothing.
>>
>> So I don't think the 'noexcept' is ever meaningful.
>>
>>
>> >> + _S_copy_chars(_CharT* __p, iterator __k1, iterator __k2)
>> >> { _S_copy_chars(__p, __k1.base(), __k2.base()); }
>> >>
>> >> - _GLIBCXX20_CONSTEXPR
>> >> static void
>> >> _S_copy_chars(_CharT* __p, const_iterator __k1, const_iterator
>> >> __k2)
>> >> - _GLIBCXX_NOEXCEPT
>> >> { _S_copy_chars(__p, __k1.base(), __k2.base()); }
>> >>
>> >> - _GLIBCXX20_CONSTEXPR
>> >> static void
>> >> - _S_copy_chars(_CharT* __p, _CharT* __k1, _CharT* __k2)
>> >> _GLIBCXX_NOEXCEPT
>> >> + _S_copy_chars(_CharT* __p, _CharT* __k1, _CharT* __k2)
>> >> { _S_copy(__p, __k1, __k2 - __k1); }
>> >>
>> >> - _GLIBCXX20_CONSTEXPR
>> >> static void
>> >> _S_copy_chars(_CharT* __p, const _CharT* __k1, const _CharT* __k2)
>> >> - _GLIBCXX_NOEXCEPT
>> >> { _S_copy(__p, __k1, __k2 - __k1); }
>> >> +#endif
>> >>
>> >> _GLIBCXX20_CONSTEXPR
>> >> static int
>> >> diff --git a/libstdc++-v3/src/c++11/string-inst.cc
>> >> b/libstdc++-v3/src/c++11/string-inst.cc
>> >> index c4864794d40..34df909b31a 100644
>> >> --- a/libstdc++-v3/src/c++11/string-inst.cc
>> >> +++ b/libstdc++-v3/src/c++11/string-inst.cc
>> >> @@ -40,7 +40,8 @@
>> >> // replaced by constrained function templates, so that we instantiate the
>> >> // pre-C++17 definitions.
>> >> // This also causes the instantiation of the non-standard C++0x-era
>> >> -// insert(iterator, initializer_list<C>) overload, see PR libstdc++/83328
>> >> +// insert(iterator, initializer_list<C>) overload, see PR
>> >> libstdc++/83328,
>> >> +// and overloads of _S_copy_chars for string iterators and pointers.
>> >> #define _GLIBCXX_DEFINING_STRING_INSTANTIATIONS 1
>> >>
>> >> #include <string>
>> >> --
>> >> 2.49.0
>> >>
>> >>
>>
