On Mon, Apr 14, 2025 at 6:08 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> 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.
>
Indeed, having a single if requires expression seems like the best option
in this situation.
>
>
> >>
> >> {
> >> 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.
>
>
