On Wed, May 27, 2026 at 5:42 PM Patrick Palka <[email protected]> wrote:
> On Wed, 27 May 2026, Tomasz Kaminski wrote: > > > > > > > On Wed, May 27, 2026 at 3:04 PM Patrick Palka <[email protected]> wrote: > > This implements the changes in sections 5, 6 and 8 of P3567R2. The > > changes in section 4 and 7 were already effectively implemented. > > > > libstdc++-v3/ChangeLog: > > > > * include/std/flat_map (_Flat_map_impl): Remove > > is_nothrow_swappable_v assertions. > > (_Flat_map_impl::_Flat_map_impl): Explicitly default copy > ctor. > > Define move ctor with corrected exception handling as per > > P3567R2. > > (_Flat_map_impl::operator=): Likewise. > > (_Flat_map_impl::insert_range): Define new __sorted_t > overload > > as per P3567R2. > > (_Flat_map_impl::swap): Make conditionally noexcept as per > > P3567R2. > > * include/std/flat_set (_Flat_set_impl): Remove > > is_nothrow_swappable_v assertion. > > (_Flat_set_impl::_Flat_set_impl): Explicitly default copy > ctor. > > Define move ctor with correct invariant preserving > behavior as > > per P3567R2. > > (_Flat_set_impl::operator=): Likewise. > > (_Flat_set_impl::swap): Make conditionally noexcept as per > > P3567R2. > > * libstdc++-v3/testsuite/23_containers/flat_map/1.cc > > (test11, test12): New tests. > > * libstdc++-v3/testsuite/23_containers/flat_multimap/1.cc > > (test11, test12): New tests. > > * libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc > > (test10): New test. > > * libstdc++-v3/testsuite/23_containers/flat_set/1.cc > > (test10): New test. > > --- > > > > Some comments follow, mostly regarding lacking guards inside a swap, > > and unrelated suggestion for optimization of insert_range for map: > > for pair-like we can extract via get direclty. > > > > libstdc++-v3/include/std/flat_map | 49 ++++++++-- > > libstdc++-v3/include/std/flat_set | 40 ++++++++- > > .../testsuite/23_containers/flat_map/1.cc | 90 > +++++++++++++++++++ > > .../23_containers/flat_multimap/1.cc | 90 > +++++++++++++++++++ > > .../23_containers/flat_multiset/1.cc | 67 ++++++++++++++ > > .../testsuite/23_containers/flat_set/1.cc | 67 ++++++++++++++ > > 6 files changed, 395 insertions(+), 8 deletions(-) > > > > diff --git a/libstdc++-v3/include/std/flat_map > b/libstdc++-v3/include/std/flat_map > > index a51c6625d54a..b42c2356474d 100644 > > --- a/libstdc++-v3/include/std/flat_map > > +++ b/libstdc++-v3/include/std/flat_map > > @@ -74,9 +74,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > static_assert(is_same_v<_Key, typename > _KeyContainer::value_type>); > > static_assert(is_same_v<_Tp, typename > _MappedContainer::value_type>); > > > > - static_assert(is_nothrow_swappable_v<_KeyContainer>); > > - static_assert(is_nothrow_swappable_v<_MappedContainer>); > > - > > using _Derived = __conditional_t<_Multi, > > flat_multimap<_Key, _Tp, > _Compare, > > > _KeyContainer, _MappedContainer>, > > @@ -395,6 +392,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > : _Flat_map_impl(__s, __il.begin(), __il.end(), __comp, > __a) > > { } > > > > + _Flat_map_impl(const _Flat_map_impl&) = default; > > + _Flat_map_impl& operator=(const _Flat_map_impl&) = default; > > + > > + _GLIBCXX26_CONSTEXPR > > + _Flat_map_impl(_Flat_map_impl&& __other) > > + noexcept(is_nothrow_move_constructible_v<containers> > > + && is_nothrow_move_constructible_v<key_compare>) > > + try > > + : _M_cont(std::move(__other._M_cont)), > _M_comp(std::move(__other._M_comp)) > > + { > > + __other.clear(); > > + } > > + catch (...) > > + { > > + __other.clear(); > > + throw; > > + } > > + > > + _GLIBCXX26_CONSTEXPR > > + _Flat_map_impl& > > + operator=(_Flat_map_impl&& __other) > > + noexcept(is_nothrow_move_assignable_v<containers> > > + && is_nothrow_move_assignable_v<key_compare>) > > + { > > + auto __guard = _M_make_clear_guard(); > > + auto __other_guard = _ClearGuard{__other._M_cont}; > > + _M_cont = std::move(__other._M_cont); > > + _M_comp = std::move(__other._M_comp); > > + __guard._M_disable(); > > + // __other_guard._M_disable is deliberately not called. > > + return *this; > > + } > > + > > _GLIBCXX26_CONSTEXPR > > _Derived& > > operator=(initializer_list<value_type> __il) > > @@ -644,6 +674,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > insert_range(_Rg&& __rg) > > { _M_insert(ranges::begin(__rg), ranges::end(__rg)); } > > > > + template<__detail::__container_compatible_range<value_type> > _Rg> > > + _GLIBCXX26_CONSTEXPR > > + void > > + insert_range(__sorted_t, _Rg&& __rg) > > + { _M_insert(ranges::begin(__rg), ranges::end(__rg), true); > } > > > > While you are touching insert_range, we could check if range_reference is > > __pair_like, and in this case move directly from the elements, instead > of creating > > a temporary value_type. But this could be a separate commit. > > Will look into that as a follow-up > > > > > + > > _GLIBCXX26_CONSTEXPR > > void > > insert(initializer_list<value_type> __il) > > @@ -728,7 +764,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > > _GLIBCXX26_CONSTEXPR > > void > > - swap(_Derived& __y) noexcept > > + swap(_Derived& __y) > > + noexcept(is_nothrow_swappable_v<key_container_type> > > + && is_nothrow_swappable_v<mapped_container_type> > > + && is_nothrow_swappable_v<key_compare>) > > { > > ranges::swap(_M_comp, __y._M_comp); > > ranges::swap(_M_cont.keys, __y._M_cont.keys); > > > > I think we break invariants here, if the keys are successfully swapped, > but > > Values are not. I think we may need to add two clear guards here, > > similarly, as they are done for the assignment in this patch. > > > > Also, I think I would keep the order the same, and move container first, > > and then comparator. > > The current order matches the Effects: clause of the standard: > https://eel.is/c++draft/flat.map#modifiers-34 > > Would you know why it's written this way? Perhaps it's better > to swap the comparator first so that if that throws we don't > have to clear keys and values. > I was thinking about it, but if the comparator swap throws, the object may no longer produce the same ordering, so we need to clear the containers anyway (keys may no longer be sorted). Going further with that, we can restore the container to a reasonable state. state after throwing, by clearing them. However, we do not have such operation for compartor, so that lead me to conclusion that it is best to touch comparator last. > > > > > @@ -974,7 +1013,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > } > > > > friend _GLIBCXX26_CONSTEXPR void > > - swap(_Derived& __x, _Derived& __y) noexcept > > + swap(_Derived& __x, _Derived& __y) > noexcept(noexcept(__x.swap(__y))) > > { return __x.swap(__y); } > > > > template<typename _Predicate> > > diff --git a/libstdc++-v3/include/std/flat_set > b/libstdc++-v3/include/std/flat_set > > index 286290896ae0..ff4a9fb2f10f 100644 > > --- a/libstdc++-v3/include/std/flat_set > > +++ b/libstdc++-v3/include/std/flat_set > > @@ -71,7 +71,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > class _Flat_set_impl > > { > > static_assert(is_same_v<_Key, typename > _KeyContainer::value_type>); > > - static_assert(is_nothrow_swappable_v<_KeyContainer>); > > > > using _Derived = __conditional_t<_Multi, > > flat_multiset<_Key, > _Compare, _KeyContainer>, > > @@ -324,6 +323,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > : _Flat_set_impl(__s, __il.begin(), __il.end(), __comp, > __a) > > { } > > > > + _Flat_set_impl(const _Flat_set_impl&) = default; > > + _Flat_set_impl& operator=(const _Flat_set_impl&) = default; > > + > > + _GLIBCXX26_CONSTEXPR > > + _Flat_set_impl(_Flat_set_impl&& __other) > > + noexcept(is_nothrow_move_constructible_v<container_type> > > + && is_nothrow_move_constructible_v<key_compare>) > > + try > > + : _M_cont(std::move(__other._M_cont)), > _M_comp(std::move(__other._M_comp)) > > + { > > + __other.clear(); > > + } > > + catch (...) > > + { > > + __other.clear(); > > + throw; > > + } > > + > > + _GLIBCXX26_CONSTEXPR > > + _Flat_set_impl& > > + operator=(_Flat_set_impl&& __other) > > + noexcept(is_nothrow_move_assignable_v<container_type> > > + && is_nothrow_move_assignable_v<key_compare>) > > + { > > + auto __guard = _M_make_clear_guard(); > > + auto __other_guard = _ClearGuard{__other._M_cont}; > > + _M_cont = std::move(__other._M_cont); > > + _M_comp = std::move(__other._M_comp); > > + __guard._M_disable(); > > + // __other_guard._M_disable is deliberately not called. > > + return *this; > > + } > > + > > _GLIBCXX26_CONSTEXPR > > _Derived& > > operator=(initializer_list<value_type> __il) > > @@ -628,7 +660,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > > _GLIBCXX26_CONSTEXPR > > void > > - swap(_Derived& __x) noexcept > > + swap(_Derived& __x) > > + noexcept(is_nothrow_swappable_v<container_type> > > + && is_nothrow_swappable_v<key_compare>) > > { > > using std::swap; > > swap(_M_cont, __x._M_cont); > > @@ -830,7 +864,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > } > > > > friend _GLIBCXX26_CONSTEXPR void > > - swap(_Derived& __x, _Derived& __y) noexcept > > + swap(_Derived& __x, _Derived& __y) > noexcept(noexcept(__x.swap(__y))) > > { return __x.swap(__y); } > > > > template<typename _Predicate> > > diff --git a/libstdc++-v3/testsuite/23_containers/flat_map/1.cc > b/libstdc++-v3/testsuite/23_containers/flat_map/1.cc > > index 1cb088f511eb..538a4bcc7766 100644 > > --- a/libstdc++-v3/testsuite/23_containers/flat_map/1.cc > > +++ b/libstdc++-v3/testsuite/23_containers/flat_map/1.cc > > @@ -299,6 +299,93 @@ test10() > > VERIFY (k == "world"); > > } > > > > +void > > +test11() > > +{ > > + static bool throw_on_move = false; > > + > > + struct vector : std::vector<int> > > + { > > + vector() = default; > > + vector(const vector&) = default; > > + vector& operator=(const vector&) = default; > > + > > + vector(vector&& other) : std::vector<int>(std::move(other)) > > + { > > + if (throw_on_move) > > + throw std::runtime_error("move ctor"); > > + } > > + > > + vector& > > + operator=(vector&& other) > > + { > > + static_cast<std::vector<int>&>(*this) = std::move(other); > > + if (throw_on_move) > > + throw std::runtime_error("move assign"); > > + return *this; > > + } > > + }; > > + > > + auto check = []<typename KC, typename MC>() { > > + using flat_map = std::flat_map<int, int, std::less<int>, KC, > MC>; > > + > > + // Verify invariant preservation upon throwing move > construction. > > + flat_map source; > > + source.insert({1, 100}); > > + source.insert({2, 200}); > > + try > > + { > > + throw_on_move = true; > > + flat_map target(std::move(source)); > > + VERIFY( false ); > > + } > > + catch (const std::runtime_error&) > > + { > > + throw_on_move = false; > > + auto extracted = std::move(source).extract(); > > > > Could you check if container itself is empty, before extracting keys and > values? > > Will do. > > > + VERIFY( extracted.keys.empty() ); > > + VERIFY( extracted.values.empty() ); > > + } > > + > > + // Verify invariant preservation upon throwing move > assignment. > > + source.clear(); > > + source.insert({1, 100}); > > + source.insert({2, 200}); > > + flat_map target; > > + target.insert({3, 300}); > > + target.insert({4, 400}); > > + try > > + { > > + throw_on_move = true; > > + target = std::move(source); > > + VERIFY( false ); > > + } > > + catch (const std::runtime_error&) > > + { > > + throw_on_move = false; > > + auto extracted = std::move(source).extract(); > > + VERIFY( extracted.keys.empty() ); > > + VERIFY( extracted.values.empty() ); > > + extracted = std::move(target).extract(); > > + VERIFY( extracted.keys.empty() ); > > + VERIFY( extracted.values.empty() ); > > + } > > + }; > > + check.template operator()<vector, std::vector<int>>(); > > + check.template operator()<std::vector<int>, vector>(); > > +} > > + > > +constexpr > > +void > > +test12() > > +{ > > + std::flat_map<int, int> m; > > + std::pair<int, int> s[] = {{1, 100}, {2, 200}}; > > + m.insert_range(std::sorted_unique, s); > > + VERIFY( std::ranges::equal(m.keys(), (int[]){1, 2}) ); > > + VERIFY( std::ranges::equal(m.values(), (int[]){100, 200}) ); > > +} > > + > > void > > test() > > { > > @@ -315,6 +402,8 @@ test() > > test08(); > > test09(); > > test10(); > > + test11(); > > + test12(); > > } > > > > constexpr > > @@ -333,6 +422,7 @@ test_constexpr() > > #if __cpp_lib_constexpr_string >= 201907L > > test10(); > > #endif > > + test12(); > > return true; > > } > > > > diff --git > a/libstdc++-v3/testsuite/23_containers/flat_multimap/1.cc > b/libstdc++-v3/testsuite/23_containers/flat_multimap/1.cc > > index 00644c14061a..14fc78318da7 100644 > > --- a/libstdc++-v3/testsuite/23_containers/flat_multimap/1.cc > > +++ b/libstdc++-v3/testsuite/23_containers/flat_multimap/1.cc > > @@ -252,6 +252,93 @@ test09() > > using value_type = std::pair<int, int>; > > } > > > > +void > > +test11() > > +{ > > + static bool throw_on_move = false; > > + > > + struct vector : std::vector<int> > > + { > > + vector() = default; > > + vector(const vector&) = default; > > + vector& operator=(const vector&) = default; > > + > > + vector(vector&& other) : std::vector<int>(std::move(other)) > > + { > > + if (throw_on_move) > > + throw std::runtime_error("move ctor"); > > + } > > + > > + vector& > > + operator=(vector&& other) > > + { > > + static_cast<std::vector<int>&>(*this) = std::move(other); > > + if (throw_on_move) > > + throw std::runtime_error("move assign"); > > + return *this; > > + } > > + }; > > + > > + auto check = []<typename KC, typename MC>() { > > + using flat_multimap = std::flat_multimap<int, int, > std::less<int>, KC, MC>; > > + > > + // Verify invariant preservation upon throwing move > construction. > > + flat_multimap source; > > + source.insert({1, 100}); > > + source.insert({2, 200}); > > + try > > + { > > + throw_on_move = true; > > + flat_multimap target(std::move(source)); > > + VERIFY( false ); > > + } > > + catch (const std::runtime_error&) > > + { > > + throw_on_move = false; > > + auto extracted = std::move(source).extract(); > > + VERIFY( extracted.keys.empty() ); > > + VERIFY( extracted.values.empty() ); > > + } > > + > > + // Verify invariant preservation upon throwing move > assignment. > > + source.clear(); > > + source.insert({1, 100}); > > + source.insert({2, 200}); > > + flat_multimap target; > > + target.insert({3, 300}); > > + target.insert({4, 400}); > > + try > > + { > > + throw_on_move = true; > > + target = std::move(source); > > + VERIFY( false ); > > + } > > + catch (const std::runtime_error&) > > + { > > + throw_on_move = false; > > + auto extracted = std::move(source).extract(); > > + VERIFY( extracted.keys.empty() ); > > + VERIFY( extracted.values.empty() ); > > + extracted = std::move(target).extract(); > > + VERIFY( extracted.keys.empty() ); > > + VERIFY( extracted.values.empty() ); > > + } > > + }; > > + check.template operator()<vector, std::vector<int>>(); > > + check.template operator()<std::vector<int>, vector>(); > > +} > > + > > +constexpr > > +void > > +test12() > > +{ > > + std::flat_multimap<int, int> m; > > + std::pair<int, int> s[] = {{1, 100}, {2, 200}}; > > + m.insert_range(std::sorted_equivalent, s); > > + VERIFY( std::ranges::equal(m.keys(), (int[]){1, 2}) ); > > + VERIFY( std::ranges::equal(m.values(), (int[]){100, 200}) ); > > +} > > + > > void > > test() > > { > > @@ -266,6 +353,8 @@ test() > > test06(); > > test07(); > > test09(); > > + test11(); > > + test12(); > > } > > > > constexpr > > @@ -280,6 +369,7 @@ test_constexpr() > > test06(); > > test07(); > > test09(); > > + test12(); > > return true; > > } > > > > diff --git > a/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc > b/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc > > index be330fefdd05..ee8b16b4ccd4 100644 > > --- a/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc > > +++ b/libstdc++-v3/testsuite/23_containers/flat_multiset/1.cc > > @@ -265,6 +265,72 @@ test09() > > VERIFY( std::ranges::equal(s, (int[]){2,2,4}) ); > > } > > > > +void > > +test10() > > +{ > > + static bool throw_on_move = false; > > + > > + struct vector : std::vector<int> > > + { > > + vector() = default; > > + vector(const vector&) = default; > > + vector& operator=(const vector&) = default; > > + > > + vector(vector&& other) : std::vector<int>(std::move(other)) > > + { > > + if (throw_on_move) > > + throw std::runtime_error("move ctor"); > > + } > > + > > + vector& > > + operator=(vector&& other) > > + { > > + static_cast<std::vector<int>&>(*this) = std::move(other); > > + if (throw_on_move) > > + throw std::runtime_error("move assign"); > > + return *this; > > + } > > + }; > > + > > + auto check = [] { > > + using flat_multiset = std::flat_multiset<int, std::less<int>, > vector>; > > + > > + // Verify invariant preservation upon throwing move > construction. > > + flat_multiset source = {1, 2}; > > + try > > + { > > + throw_on_move = true; > > + flat_multiset target(std::move(source)); > > + VERIFY( false ); > > + } > > + catch (const std::runtime_error&) > > + { > > + throw_on_move = false; > > + auto extracted = std::move(source).extract(); > > + VERIFY( extracted.empty() ); > > + } > > + > > + // Verify invariant preservation upon throwing move > assignment. > > + source = {1, 2}; > > + flat_multiset target = {3, 4}; > > + try > > + { > > + throw_on_move = true; > > + target = std::move(source); > > + VERIFY( false ); > > + } > > + catch (const std::runtime_error&) > > + { > > + throw_on_move = false; > > + auto extracted = std::move(source).extract(); > > + VERIFY( extracted.empty() ); > > + extracted = std::move(target).extract(); > > + VERIFY( extracted.empty() ); > > + } > > + }; > > + check(); > > +} > > + > > void > > test() > > { > > @@ -278,6 +344,7 @@ test() > > test07(); > > test08(); > > test09(); > > + test10(); > > } > > > > constexpr > > diff --git a/libstdc++-v3/testsuite/23_containers/flat_set/1.cc > b/libstdc++-v3/testsuite/23_containers/flat_set/1.cc > > index a14d03476e18..2f08d25a714e 100644 > > --- a/libstdc++-v3/testsuite/23_containers/flat_set/1.cc > > +++ b/libstdc++-v3/testsuite/23_containers/flat_set/1.cc > > @@ -277,6 +277,72 @@ test09() > > VERIFY( std::ranges::equal(s, (int[]){2,4}) ); > > } > > > > +void > > +test10() > > +{ > > + static bool throw_on_move = false; > > + > > + struct vector : std::vector<int> > > + { > > + vector() = default; > > + vector(const vector&) = default; > > + vector& operator=(const vector&) = default; > > + > > + vector(vector&& other) : std::vector<int>(std::move(other)) > > + { > > + if (throw_on_move) > > + throw std::runtime_error("move ctor"); > > + } > > + > > + vector& > > + operator=(vector&& other) > > + { > > + static_cast<std::vector<int>&>(*this) = std::move(other); > > + if (throw_on_move) > > + throw std::runtime_error("move assign"); > > + return *this; > > + } > > + }; > > + > > + auto check = [] { > > + using flat_set = std::flat_set<int, std::less<int>, vector>; > > + > > + // Verify invariant preservation upon throwing move > construction. > > + flat_set source = {1, 2}; > > + try > > + { > > + throw_on_move = true; > > + flat_set target(std::move(source)); > > + VERIFY( false ); > > + } > > + catch (const std::runtime_error&) > > + { > > + throw_on_move = false; > > + auto extracted = std::move(source).extract(); > > + VERIFY( extracted.empty() ); > > + } > > + > > + // Verify invariant preservation upon throwing move > assignment. > > + source = {1, 2}; > > + flat_set target = {3, 4}; > > + try > > + { > > + throw_on_move = true; > > + target = std::move(source); > > + VERIFY( false ); > > + } > > + catch (const std::runtime_error&) > > + { > > + throw_on_move = false; > > + auto extracted = std::move(source).extract(); > > + VERIFY( extracted.empty() ); > > + extracted = std::move(target).extract(); > > + VERIFY( extracted.empty() ); > > + } > > + }; > > + check(); > > +} > > + > > void > > test() > > { > > @@ -290,6 +356,7 @@ test() > > test07(); > > test08(); > > test09(); > > + test10(); > > } > > > > constexpr > > -- > > 2.54.0.rc1.54.g60f07c4f5c > > > > > > >
