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