On Mon, 28 Apr 2025 at 16:22, Patrick Palka <ppa...@redhat.com> wrote:
>
> On Mon, 28 Apr 2025, Jonathan Wakely wrote:
>
> > On Mon, 28 Apr 2025 at 14:59, Patrick Palka <ppa...@redhat.com> wrote:
> > >
> > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk/15?
> > >
> > > -- >8 --
> > >
> > > These std::erase_if overloads were wrongly implemented as hidden
> > > friends, visible only via ADL, so erase_if(x) would work but not
> > > std::erase_if(x).
> >
> > Would it be easier to just add a namespace-scope declaration of the
> > friend, so they're not hidden? That would avoid needing both a private
> > member *and* a friend.
>
> I don't think that'd work in this case since for a friend definition
> inside a class template, each instantiation of the class template will
> produce a logically distinct erase_if overload, but here we want just
> four overloads (one for flat_map, flat_multimap, flat_set,
> flat_multiset), to match the four namespace-scope declarations.
Ack.
> I think we can do the opposite, and define these erase_if's directly at
> namespace scope and just declare them as friends, to get rid of
> _M_erase_if, but the member function lets us write a single
> implementation that works for both flat_foo and flat_multifoo, instead
> of having to duplicate it, so the member function approach seems overall
> more concise.
Ah yes, makes sense, thanks.
OK for trunk and 15.
> >
> > >
> > > PR libstdc++/119427
> > >
> > > libstdc++-v3/ChangeLog:
> > >
> > > * include/std/flat_map (_Flat_map_impl::erase_if): Replace
> > > this hidden friend with ...
> > > (_Flat_map_impl::_M_erase_if): ... this member function.
> > > (flat_map): Export _Flat_map_impl::_M_erase_if.
> > > (flat_multimap): Likewise.
> > > * include/std/flat_set (_Flat_set_impl::erase_if): Replace
> > > with ...
> > > (_Flat_set_impl::_M_erase_if): ... this member function.
> > > (flat_set): Export _Flat_set_impl::_M_erase_if.
> > > (flat_multiset): Likewise.
> > > * testsuite/23_containers/flat_map/1.c (test07): New test.
> > > * testsuite/23_containers/flat_multimap/1.cc (test07): New test.
> > > * testsuite/23_containers/flat_multiset/1.cc (test09): New test.
> > > * testsuite/23_containers/flat_set/1.cc (test09): New test.
> > > ---
> > > libstdc++-v3/include/std/flat_map | 28 +++++++++++++++----
> > > libstdc++-v3/include/std/flat_set | 28 +++++++++++++++----
> > > .../testsuite/23_containers/flat_map/1.cc | 11 ++++++++
> > > .../23_containers/flat_multimap/1.cc | 11 ++++++++
> > > .../23_containers/flat_multiset/1.cc | 11 ++++++++
> > > .../testsuite/23_containers/flat_set/1.cc | 11 ++++++++
> > > 6 files changed, 89 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/libstdc++-v3/include/std/flat_map
> > > b/libstdc++-v3/include/std/flat_map
> > > index 405caa8a81bf..6593988d213c 100644
> > > --- a/libstdc++-v3/include/std/flat_map
> > > +++ b/libstdc++-v3/include/std/flat_map
> > > @@ -890,14 +890,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > { return __x.swap(__y); }
> > >
> > > template<typename _Predicate>
> > > - friend size_type
> > > - erase_if(_Derived& __c, _Predicate __pred)
> > > + size_type
> > > + _M_erase_if(_Predicate __pred)
> > > {
> > > - auto __guard = __c._M_make_clear_guard();
> > > - auto __zv = views::zip(__c._M_cont.keys, __c._M_cont.values);
> > > + auto __guard = _M_make_clear_guard();
> > > + auto __zv = views::zip(_M_cont.keys, _M_cont.values);
> > > auto __sr = ranges::remove_if(__zv, __pred);
> > > auto __erased = __sr.size();
> > > - __c.erase(__c.end() - __erased, __c.end());
> > > + erase(end() - __erased, end());
> > > __guard._M_disable();
> > > return __erased;
> > > }
> > > @@ -1329,6 +1329,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > using _Impl::lower_bound;
> > > using _Impl::upper_bound;
> > > using _Impl::equal_range;
> > > +
> > > + using _Impl::_M_erase_if;
> > > };
> > >
> > > template<typename _KeyContainer, typename _MappedContainer,
> > > @@ -1412,6 +1414,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > && uses_allocator_v<_MappedContainer, _Alloc>>
> > > { };
> > >
> > > + template<typename _Key, typename _Tp, typename _Compare,
> > > + typename _KeyContainer, typename _MappedContainer, typename
> > > _Predicate>
> > > + typename flat_map<_Key, _Tp, _Compare, _KeyContainer,
> > > _MappedContainer>::size_type
> > > + erase_if(flat_map<_Key, _Tp, _Compare, _KeyContainer,
> > > _MappedContainer>& __c,
> > > + _Predicate __pred)
> > > + { return __c._M_erase_if(std::move(__pred)); }
> > > +
> > > /* Class template flat_multimap - container adaptor
> > > *
> > > * @ingroup
> > > @@ -1487,6 +1496,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > using _Impl::lower_bound;
> > > using _Impl::upper_bound;
> > > using _Impl::equal_range;
> > > +
> > > + using _Impl::_M_erase_if;
> > > };
> > >
> > > template<typename _KeyContainer, typename _MappedContainer,
> > > @@ -1571,6 +1582,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > && uses_allocator_v<_MappedContainer, _Alloc>>
> > > { };
> > >
> > > + template<typename _Key, typename _Tp, typename _Compare,
> > > + typename _KeyContainer, typename _MappedContainer, typename
> > > _Predicate>
> > > + typename flat_multimap<_Key, _Tp, _Compare, _KeyContainer,
> > > _MappedContainer>::size_type
> > > + erase_if(flat_multimap<_Key, _Tp, _Compare, _KeyContainer,
> > > _MappedContainer>& __c,
> > > + _Predicate __pred)
> > > + { return __c._M_erase_if(std::move(__pred)); }
> > > +
> > > _GLIBCXX_END_NAMESPACE_VERSION
> > > } // namespace std
> > > #endif // __cpp_lib_flat_map
> > > diff --git a/libstdc++-v3/include/std/flat_set
> > > b/libstdc++-v3/include/std/flat_set
> > > index 3e15d1af4162..c48340d79809 100644
> > > --- a/libstdc++-v3/include/std/flat_set
> > > +++ b/libstdc++-v3/include/std/flat_set
> > > @@ -745,15 +745,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > { return __x.swap(__y); }
> > >
> > > template<typename _Predicate>
> > > - friend size_type
> > > - erase_if(_Derived& __c, _Predicate __pred)
> > > + size_type
> > > + _M_erase_if(_Predicate __pred)
> > > {
> > > - auto __guard = __c._M_make_clear_guard();
> > > - auto __first = __c._M_cont.begin();
> > > - auto __last = __c._M_cont.end();
> > > + auto __guard = _M_make_clear_guard();
> > > + auto __first = _M_cont.begin();
> > > + auto __last = _M_cont.end();
> > > __first = std::remove_if(__first, __last, __pred);
> > > auto __n = __last - __first;
> > > - __c.erase(__first, __last);
> > > + erase(__first, __last);
> > > __guard._M_disable();
> > > return __n;
> > > }
> > > @@ -860,6 +860,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > using _Impl::lower_bound;
> > > using _Impl::upper_bound;
> > > using _Impl::equal_range;
> > > +
> > > + using _Impl::_M_erase_if;
> > > };
> > >
> > > template<typename _KeyContainer,
> > > @@ -930,6 +932,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > : bool_constant<uses_allocator_v<_KeyContainer, _Alloc>>
> > > { };
> > >
> > > + template<typename _Key, typename _Compare, typename _KeyContainer,
> > > + typename _Predicate>
> > > + typename flat_set<_Key, _Compare, _KeyContainer>::size_type
> > > + erase_if(flat_set<_Key, _Compare, _KeyContainer>& __c, _Predicate
> > > __pred)
> > > + { return __c._M_erase_if(std::move(__pred)); }
> > > +
> > > /* Class template flat_multiset - container adaptor
> > > *
> > > * @ingroup
> > > @@ -999,6 +1007,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > using _Impl::lower_bound;
> > > using _Impl::upper_bound;
> > > using _Impl::equal_range;
> > > +
> > > + using _Impl::_M_erase_if;
> > > };
> > >
> > > template<typename _KeyContainer,
> > > @@ -1069,6 +1079,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > : bool_constant<uses_allocator_v<_KeyContainer, _Alloc>>
> > > { };
> > >
> > > + template<typename _Key, typename _Compare, typename _KeyContainer,
> > > + typename _Predicate>
> > > + typename flat_multiset<_Key, _Compare, _KeyContainer>::size_type
> > > + erase_if(flat_multiset<_Key, _Compare, _KeyContainer>& __c,
> > > _Predicate __pred)
> > > + { return __c._M_erase_if(std::move(__pred)); }
> > > +
> > > _GLIBCXX_END_NAMESPACE_VERSION
> > > } // namespace std
> > > #endif // __cpp_lib_flat_set
> > > diff --git a/libstdc++-v3/testsuite/23_containers/flat_map/1.cc
> > > b/libstdc++-v3/testsuite/23_containers/flat_map/1.cc
> > > index 4fd33f616f78..2e96dc0c8ea4 100644
> > > --- a/libstdc++-v3/testsuite/23_containers/flat_map/1.cc
> > > +++ b/libstdc++-v3/testsuite/23_containers/flat_map/1.cc
> > > @@ -242,6 +242,16 @@ test06()
> > > VERIFY( std::ranges::equal(m | std::views::values, (int[]){2, 3, 4, 5,
> > > 6}) );
> > > }
> > >
> > > +void
> > > +test07()
> > > +{
> > > + // PR libstdc++/119427 - std::erase_if(std::flat_foo) does not work
> > > + std::flat_map<int, int> m = {std::pair{1, 2}, {3, 4}, {5, 6}};
> > > + auto n = std::erase_if(m, [](auto x) { auto [k,v] = x; return k == 1
> > > || v == 6; });
> > > + VERIFY( n == 2 );
> > > + VERIFY( std::ranges::equal(m, (std::pair<int,int>[]){{3,4}}) );
> > > +}
> > > +
> > > int
> > > main()
> > > {
> > > @@ -254,4 +264,5 @@ main()
> > > test04();
> > > test05();
> > > test06();
> > > + test07();
> > > }
> > > diff --git a/libstdc++-v3/testsuite/23_containers/flat_multimap/1.cc
> > > b/libstdc++-v3/testsuite/23_containers/flat_multimap/1.cc
> > > index ea0d4b41e9fb..13919b44b6e4 100644
> > > --- a/libstdc++-v3/testsuite/23_containers/flat_multimap/1.cc
> > > +++ b/libstdc++-v3/testsuite/23_containers/flat_multimap/1.cc
> > > @@ -220,6 +220,16 @@ test06()
> > > VERIFY( std::ranges::equal(m | std::views::values, (int[]){2, 3, 4, 5,
> > > 6}) );
> > > }
> > >
> > > +void
> > > +test07()
> > > +{
> > > + // PR libstdc++/119427 - std::erase_if(std::flat_foo) does not work
> > > + std::flat_multimap<int, int> m = {std::pair{1, 2}, {3, 4}, {3, 3}, {5,
> > > 6}, {6, 6}};
> > > + auto n = std::erase_if(m, [](auto x) { auto [k,v] = x; return k == 1
> > > || v == 6; });
> > > + VERIFY( n == 3 );
> > > + VERIFY( std::ranges::equal(m, (std::pair<int,int>[]){{3,4},{3,3}}) );
> > > +}
> > > +
> > > int
> > > main()
> > > {
> > > @@ -232,4 +242,5 @@ main()
> > > test04();
> > > test05();
> > > test06();
> > > + test07();
> > > }
> > > diff --git a/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc
> > > b/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc
> > > index 7d9a33c6b000..63855e070650 100644
> > > --- a/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc
> > > +++ b/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc
> > > @@ -235,6 +235,16 @@ test08()
> > > VERIFY( copy_counter == 2 );
> > > }
> > >
> > > +void
> > > +test09()
> > > +{
> > > + // PR libstdc++/119427 - std::erase_if(std::flat_foo) does not work
> > > + std::flat_multiset<int> s = {1,1,2,2,3,4,5};
> > > + auto n = std::erase_if(s, [](int x) { return x % 2 != 0; });
> > > + VERIFY( n == 4 );
> > > + VERIFY( std::ranges::equal(s, (int[]){2,2,4}) );
> > > +}
> > > +
> > > int
> > > main()
> > > {
> > > @@ -247,4 +257,5 @@ main()
> > > test06();
> > > test07();
> > > test08();
> > > + test09();
> > > }
> > > diff --git a/libstdc++-v3/testsuite/23_containers/flat_set/1.cc
> > > b/libstdc++-v3/testsuite/23_containers/flat_set/1.cc
> > > index ed24fab785b5..b1d9002caba3 100644
> > > --- a/libstdc++-v3/testsuite/23_containers/flat_set/1.cc
> > > +++ b/libstdc++-v3/testsuite/23_containers/flat_set/1.cc
> > > @@ -248,6 +248,16 @@ test08()
> > > VERIFY( copy_counter == 1 );
> > > }
> > >
> > > +void
> > > +test09()
> > > +{
> > > + // PR libstdc++/119427 - std::erase_if(std::flat_foo) does not work
> > > + std::flat_set<int> s = {1,2,3,4,5};
> > > + auto n = std::erase_if(s, [](int x) { return x % 2 != 0; });
> > > + VERIFY( n == 3 );
> > > + VERIFY( std::ranges::equal(s, (int[]){2,4}) );
> > > +}
> > > +
> > > int
> > > main()
> > > {
> > > @@ -260,4 +270,5 @@ main()
> > > test06();
> > > test07();
> > > test08();
> > > + test09();
> > > }
> > > --
> > > 2.49.0.459.gf65182a99e
> > >
> >
> >
>
