On Mon, 24 Mar 2025 at 17:47, Patrick Palka <ppa...@redhat.com> wrote:
>
> On Mon, 24 Mar 2025, Tomasz Kamiński wrote:
>
> > These patch add check to verify if common range iterators satisfies
> > Cpp17LegacyIterator requirements (__detail::__cpp17_input_iterator),
> > before invoking overloads of insert that accepts two iterators.
> > As such overloads existed before c++20 iterators were introduced,
> > they commonly assume existence of iterator_traits<..>::iterator_category,
> > and passing a cpp20-only iterators, leads to hard errors.
> >
> > In case if user-defined container wants to support more efficient
> > insertion in such cases, it should provided insert_range method,
> > as in the case of standard containers.
> >
> > PR libstdc++/119415
> >
> > libstdc++-v3/ChangeLog:
> >
> > * include/std/flat_set (flat_multiset::insert_range,
> > flat_set::insert_range): Add __detail::__cpp17_input_iterator check.
>
> There's a chance this ChangeLog entry might not be accepted by the
> pre-commit hook. It might need to be written as
>
> * include/std/flat_set (flat_multiset::insert_range)
> (flat_set::insert_range): Add __detail::__cpp17_input_iterator check.
>
> as per the ChangeLog style guide[1] which says:
>
> Break long lists of function names by closing continued lines with ‘)’,
> rather than ‘,’, and opening the continuation with ‘(’.
>
> OTOH, what the patch is lexically changing is _Flat_set_impl::insert_range,
> so maybe the ChangeLog entry should just mention that?
>
> [1]:
> https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html#Style-of-Change-Logs;
OK for trunk with Patrick's comments addressed.
>
> > * testsuite/23_containers/flat_multiset/1.cc: New tests
> > * testsuite/23_containers/flat_set/1.cc: New tests
> > ---
> > Testing on x86_64-linux. OK for trunk?
> >
> > libstdc++-v3/include/std/flat_set | 3 +-
> > .../23_containers/flat_multiset/1.cc | 18 ++++++++++++
> > .../testsuite/23_containers/flat_set/1.cc | 28 +++++++++++++++++++
> > 3 files changed, 48 insertions(+), 1 deletion(-)
> >
> > diff --git a/libstdc++-v3/include/std/flat_set
> > b/libstdc++-v3/include/std/flat_set
> > index 9240f2b9a2e..ea416e67503 100644
> > --- a/libstdc++-v3/include/std/flat_set
> > +++ b/libstdc++-v3/include/std/flat_set
> > @@ -480,7 +480,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > typename container_type::iterator __it;
> > if constexpr (requires { _M_cont.insert_range(_M_cont.end(), __rg);
> > })
> > __it = _M_cont.insert_range(_M_cont.end(), __rg);
> > - else if constexpr (ranges::common_range<_Rg>)
> > + else if constexpr (ranges::common_range<_Rg>
> > + &&
> > __detail::__cpp17_input_iterator<ranges::iterator_t<_Rg>>)
> > __it = _M_cont.insert(_M_cont.end(), ranges::begin(__rg),
> > ranges::end(__rg));
> > else
> > {
> > diff --git a/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc
> > b/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc
> > index 910f5dca5be..0858be4a9fe 100644
> > --- a/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc
> > +++ b/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc
> > @@ -143,6 +143,23 @@ test06()
> > VERIFY( std::ranges::equal(s, (int[]){1, 2, 3, 4, 5}) );
> > }
> >
> > +void test07()
> > +{
> > +#ifdef __SIZEOF_INT128__
> > + // PR libstdc++/119415 - flat_foo::insert_range cannot handle common
> > ranges
> > + // on c++20 only iterators
> > + auto r = std::views::iota(__int128(1), __int128(6));
> > +
> > + std::flat_multiset<int> s;
> > + s.insert_range(r);
> > + VERIFY( std::ranges::equal(s, (int[]){1, 2, 3, 4, 5}) );
> > +
> > + std::flat_multiset<int, std::less<int>, NoInsertRange<int>> s2;
> > + s2.insert_range(r);
> > + VERIFY( std::ranges::equal(s2, (int[]){1, 2, 3, 4, 5}) );
> > +#endif
> > +}
> > +
> > int
> > main()
> > {
> > @@ -153,4 +170,5 @@ main()
> > test04();
> > test05();
> > test06();
> > + test07();
> > }
> > diff --git a/libstdc++-v3/testsuite/23_containers/flat_set/1.cc
> > b/libstdc++-v3/testsuite/23_containers/flat_set/1.cc
> > index f0eaac936bf..374be80bb9d 100644
> > --- a/libstdc++-v3/testsuite/23_containers/flat_set/1.cc
> > +++ b/libstdc++-v3/testsuite/23_containers/flat_set/1.cc
> > @@ -8,6 +8,7 @@
> >
> > #include <deque>
> > #include <ranges>
> > +#include <spanstream>
> > #include <vector>
> > #include <testsuite_allocator.h>
> > #include <testsuite_hooks.h>
> > @@ -158,6 +159,32 @@ test06()
> > VERIFY( std::ranges::equal(s, (int[]){1, 2, 3, 4, 5}) );
> > }
> >
> > +template<typename T>
> > +struct NoInsertRange : std::vector<T>
> > +{
> > + using std::vector<T>::vector;
> > +
> > + template<typename It, typename R>
> > + void insert_range(typename std::vector<T>::const_iterator, R&&) = delete;
> > +};
> > +
> > +void test07()
> > +{
> > +#ifdef __SIZEOF_INT128__
> > + // PR libstdc++/119415 - flat_foo::insert_range cannot handle common
> > ranges
> > + // on c++20 only iterators
> > + auto r = std::views::iota(__int128(1), __int128(6));
> > +
> > + std::flat_set<int> s;
> > + s.insert_range(r);
> > + VERIFY( std::ranges::equal(s, (int[]){1, 2, 3, 4, 5}) );
> > +
> > + std::flat_set<int, std::less<int>, NoInsertRange<int>> s2;
> > + s2.insert_range(r);
> > + VERIFY( std::ranges::equal(s2, (int[]){1, 2, 3, 4, 5}) );
> > +#endif
> > +}
> > +
> > int
> > main()
> > {
> > @@ -168,4 +195,5 @@ main()
> > test04();
> > test05();
> > test06();
> > + test07();
> > }
> > --
> > 2.48.1
> >
> >
