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.
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.
>
> >
> > 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
> >
>
>
