On Mon, Apr 14, 2025 at 5:06 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> My recent r15-9381-g648d5c26e25497 change assumes that a contiguous
> iterator with the correct value_type can be converted to a const charT*
> but that's not true for volatile charT*. The optimization should only be
> done if it can be converted to the right pointer type.
>
> Additionally, the generic loop for non-contiguous iterators needs an
> explicit cast to deal with iterators that have a reference type that is
> not explicitly convertible to charT.
>
As mentioned before, we do not need to handle explicit conversion for CharT.
We still need to perform static_cast to create a temporary CharT when
passing
to traits::assign, as otherwise we will get a mismatch in deduction.
>
> libstdc++-v3/ChangeLog:
>
> PR libstdc++/119748
> * include/bits/basic_string.h (_S_copy_chars): Only optimize for
> contiguous iterators that are convertible to const charT*. Use
> explicit conversion to charT after dereferencing iterator.
> (_S_copy_range): Likewise for contiguous ranges.
> * include/bits/basic_string.tcc (_M_construct): Use explicit
> conversion to charT after dereferencing input iterator.
> * include/bits/cow_string.h (_S_copy_chars): Likewise.
> (basic_string(from_range_t, R&&, const Allocator&)): Likewise.
> Only optimize for contiguous iterators that are convertible to
> const charT*.
> * testsuite/21_strings/basic_string/cons/char/119748.cc: New
> test.
> * testsuite/21_strings/basic_string/cons/wchar_t/119748.cc:
> New test.
> ---
>
> Changes in v2:
> - Added static_assert in _M_construct overload for input iterators.
> - Added tests using non-contiguous iterators.
>
> libstdc++-v3/include/bits/basic_string.h | 24 +++++---
> libstdc++-v3/include/bits/basic_string.tcc | 3 +-
> libstdc++-v3/include/bits/cow_string.h | 17 ++++--
> .../basic_string/cons/char/119748.cc | 55 +++++++++++++++++++
> .../basic_string/cons/wchar_t/119748.cc | 7 +++
> 5 files changed, 93 insertions(+), 13 deletions(-)
> create mode 100644
> libstdc++-v3/testsuite/21_strings/basic_string/cons/char/119748.cc
> create mode 100644
> libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/119748.cc
>
> diff --git a/libstdc++-v3/include/bits/basic_string.h
> b/libstdc++-v3/include/bits/basic_string.h
> index 9c431c765ab..c90bd099b63 100644
> --- a/libstdc++-v3/include/bits/basic_string.h
> +++ b/libstdc++-v3/include/bits/basic_string.h
> @@ -488,8 +488,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> 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>)
> + else if constexpr (requires {
> + requires contiguous_iterator<_Iterator>;
> + { std::to_address(__k1) }
> + -> convertible_to<const _CharT*>;
> + })
>
I would prefer, but this is not a strong preference.
if constexpr (ranges::contiguous_range<_Rg>)
if constexpr
(convertible_to<decltype(ranges::data(std::forward<_Rg>(__rg))), const
CharT*>>)
> {
> const auto __d = __k2 - __k1;
> (void) (__k1 + __d); // See P3349R1
> @@ -499,7 +502,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> else
> #endif
> for (; __k1 != __k2; ++__k1, (void)++__p)
> - traits_type::assign(*__p, *__k1); // These types are off.
> + traits_type::assign(*__p, static_cast<_CharT>(*__k1));
> }
> #pragma GCC diagnostic pop
>
> @@ -527,12 +530,19 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> static constexpr void
> _S_copy_range(pointer __p, _Rg&& __rg, size_type __n)
> {
> - if constexpr (ranges::contiguous_range<_Rg>
> - && is_same_v<ranges::range_value_t<_Rg>, _CharT>)
> + if constexpr (requires {
> + requires ranges::contiguous_range<_Rg>;
> + { ranges::data(std::forward<_Rg>(__rg)) }
> + -> convertible_to<const _CharT*>;
>
Same as before.
+ })
> _S_copy(__p, ranges::data(std::forward<_Rg>(__rg)), __n);
> else
> - for (auto&& __e : __rg)
> - traits_type::assign(*__p++,
> std::forward<decltype(__e)>(__e));
> + {
> + auto __first = ranges::begin(__rg);
> + const auto __last = ranges::end(__rg);
> + for (; __first != __last; ++__first)
> + traits_type::assign(*__p++, static_cast<_CharT>(*__first));
> + }
> }
> #endif
>
> diff --git a/libstdc++-v3/include/bits/basic_string.tcc
> b/libstdc++-v3/include/bits/basic_string.tcc
> index 02230aca5d2..bca55bc5658 100644
> --- a/libstdc++-v3/include/bits/basic_string.tcc
> +++ b/libstdc++-v3/include/bits/basic_string.tcc
> @@ -210,7 +210,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _M_data(__another);
> _M_capacity(__capacity);
> }
> - traits_type::assign(_M_data()[__len++], *__beg);
> + traits_type::assign(_M_data()[__len++],
> + static_cast<_CharT>(*__beg));
> ++__beg;
> }
>
> diff --git a/libstdc++-v3/include/bits/cow_string.h
> b/libstdc++-v3/include/bits/cow_string.h
> index b250397151b..f9df2be20be 100644
> --- a/libstdc++-v3/include/bits/cow_string.h
> +++ b/libstdc++-v3/include/bits/cow_string.h
> @@ -423,7 +423,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _S_copy_chars(_CharT* __p, _Iterator __k1, _Iterator __k2)
> {
> for (; __k1 != __k2; ++__k1, (void)++__p)
> - traits_type::assign(*__p, *__k1); // These types are off.
> + traits_type::assign(*__p, static_cast<_CharT>(*__k1));
> }
>
> static void
> @@ -656,12 +656,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> reserve(__n);
> pointer __p = _M_data();
> - if constexpr (ranges::contiguous_range<_Rg>
> - && is_same_v<ranges::range_value_t<_Rg>,
> _CharT>)
> + if constexpr (requires {
> + requires ranges::contiguous_range<_Rg>;
> + { ranges::data(std::forward<_Rg>(__rg)) }
> + -> convertible_to<const _CharT*>;
> + })
> _M_copy(__p, ranges::data(std::forward<_Rg>(__rg)), __n);
> else
> - for (auto&& __e : __rg)
> - traits_type::assign(*__p++,
> std::forward<decltype(__e)>(__e));
> + {
> + auto __first = ranges::begin(__rg);
> + const auto __last = ranges::end(__rg);
> + for (; __first != __last; ++__first)
> + traits_type::assign(*__p++,
> static_cast<_CharT>(*__first));
> + }
> _M_rep()->_M_set_length_and_sharable(__n);
> }
> else
> diff --git
> a/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/119748.cc
> b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/119748.cc
> new file mode 100644
> index 00000000000..c8be6f4ec0a
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/119748.cc
> @@ -0,0 +1,55 @@
> +// { dg-do compile }
> +
> +// Bug 119748
> +// string(InputIterator, InputIterator) rejects volatile charT* as
> iterator
> +
> +#ifndef TEST_CHAR_TYPE
> +#define TEST_CHAR_TYPE char
> +#endif
> +
> +#include <string>
> +
> +typedef TEST_CHAR_TYPE C;
> +
> +volatile C vs[42] = {};
> +std::basic_string<C> s(vs+0, vs+42);
> +#ifdef __cpp_lib_containers_ranges
> +std::basic_string<C> s2(std::from_range, vs);
> +#endif
> +
> +template<class Cat>
> +struct Iterator
> +{
> + typedef C value_type;
> + typedef volatile C& reference;
> + typedef volatile C* pointer;
> + typedef long difference_type;
> + typedef Cat iterator_category;
> +
> + reference operator*() const { return const_cast<reference>(*c); };
> +
> + Iterator& operator++() { ++c; return *this; }
> + Iterator operator++(int) { Iterator i = { c++ }; return i; }
> +
> + bool operator==(const Iterator& i) const { return c == i.c; }
> + bool operator!=(const Iterator& i) const { return !(*this == i); }
> +
> + C* c;
> +};
> +
> +typedef Iterator<std::input_iterator_tag> InputIterator;
>
Any reason for creating own iterators, instead relying on one from
test_iterators.h?
> +C chars[] = { C('a'), C('b'), C('c') };
> +InputIterator first = { chars + 0 };
> +InputIterator last = { chars + 3 };
> +std::basic_string<C> s3(first, last);
> +#ifdef __cpp_lib_containers_ranges
> +std::basic_string<C> s4(std::from_range, std::ranges::subrange(first,
> last));
> +#endif
> +
> +typedef Iterator<std::forward_iterator_tag> ForwardIterator;
> +ForwardIterator firstf = { chars+0 };
> +ForwardIterator lastf = { chars + 3 };
> +std::basic_string<C> s5(firstf, lastf);
> +#ifdef __cpp_lib_containers_ranges
> +std::basic_string<C> s6(std::from_range, std::ranges::subrange(firstf,
> lastf));
> +#endif
> diff --git
> a/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/119748.cc
> b/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/119748.cc
> new file mode 100644
> index 00000000000..7d3ba10d2d4
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/119748.cc
> @@ -0,0 +1,7 @@
> +// { dg-do compile }
> +
> +// Bug 119748
> +// string(InputIterator, InputIterator) rejects volatile charT* as
> iterator
> +
> +#define TEST_CHAR_TYPE wchar_t
> +#include "../char/119748.cc"
> --
> 2.49.0
>
>
