On Mon, 14 Apr 2025 at 16:25, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> 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.
Ah yes, I forgot to fix that part of the commit message.
> 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.
I agree we still need to create a temporary, but there's no deduction
involved. traits_type::assign takes charT& and const charT&, and the
volatile charT& cannot bind to the latter.
I'll change the second paragraph of the commit message to say:
Additionally, some generic loops for non-contiguous iterators need an
explicit cast to deal with iterator reference types that do not bind to
the const charT& parameter of traits_type::assign.
>> 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*>>)
Yes, in general I would prefer that, but then the else-branch would
bind incorrectly:
if constexpr (pointer or string::iterator)
{ }
else if constexpr (contiguous_range<R>)
if constexpr (convertible to pointer)
{ }
else
for-loop
And it can't be a single condition because the ranges::data call isn't
valid unless it's a contiguous iterator.
I could write it:
if constexpr (pointer or string::iterator)
{ }
else if constexpr (contiguous_range<R>)
if constexpr (convertible to pointer)
{ }
else
for-loop
else
for-loop
Or just don't use a discarded statement for the for-loop:
if constexpr (pointer or string::iterator)
{ }
else if constexpr (contiguous_range<R>)
if constexpr (convertible to pointer)
{ }
for-loop
But neither of these seems clearly better than just using a single
constexpr-if with a requires-expression as the condition.
>>
>> {
>> 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?
I thought they didn't work with volatile pointers, but it looks like I
was wrong.
