From c15cecb8637d31f136e971d25a6b50b42e49aa03 Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Sat, 28 Oct 2023 03:52:58 +0800 Subject: [PATCH 1/7] 1. add test for set requirements --- tests/std/tests/P1222R4_flat_set/test.cpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/std/tests/P1222R4_flat_set/test.cpp b/tests/std/tests/P1222R4_flat_set/test.cpp index e1976302b37..3586eb40705 100644 --- a/tests/std/tests/P1222R4_flat_set/test.cpp +++ b/tests/std/tests/P1222R4_flat_set/test.cpp @@ -33,7 +33,7 @@ void assert_container_requirements(const T& s) { static_assert(is_same_v m.end()), strong_ordering>); static_assert(is_same_v); static_assert(is_same_v); - static_assert(is_same_v); + static_assert(is_same_v); static_assert(is_same_v); T my_moved = std::move(m); @@ -73,6 +73,22 @@ void assert_reversible_container_requirements(const T& s) { static_assert(is_convertible_v); } +template +void assert_set_requirements() { + using iterator = T::iterator; + using const_iterator = T::const_iterator; + using key_type = T::key_type; + using value_type = T::value_type; + + static_assert(_Constant_iterator); + static_assert(is_convertible_v); + + // additionally: + static_assert(is_same_v); + static_assert(_Constant_iterator); + static_assert(is_convertible_v); +} + template void assert_noexcept_requirements(T& s) { static_assert(noexcept(s.begin())); @@ -99,6 +115,7 @@ template void assert_all_requirements(const T& s) { assert_container_requirements(s); assert_reversible_container_requirements(s); + assert_set_requirements(); assert_noexcept_requirements(s); assert_noexcept_requirements(const_cast(s)); From 39eee7bbec096dbd189950822f741ee1a41db305 Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Sat, 28 Oct 2023 04:41:11 +0800 Subject: [PATCH 2/7] 2. fixes --- stl/inc/flat_set | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/stl/inc/flat_set b/stl/inc/flat_set index d31bf71ce01..74fb4a0a6b1 100644 --- a/stl/inc/flat_set +++ b/stl/inc/flat_set @@ -56,8 +56,8 @@ public: using const_reference = const value_type&; using size_type = _Container::size_type; using difference_type = _Container::difference_type; - using iterator = _Container::iterator; using const_iterator = _Container::const_iterator; + using iterator = const_iterator; using reverse_iterator = _STD reverse_iterator; using const_reverse_iterator = _STD reverse_iterator; using container_type = _Container; @@ -324,7 +324,9 @@ public: _Guard._Target = nullptr; } - iterator erase(iterator _Where) { + iterator erase(iterator _Where) + requires (!is_same_v) + { return _Mycont.erase(_Where); } iterator erase(const_iterator _Where) { @@ -359,6 +361,10 @@ public: return _Mycomp; } + _NODISCARD const container_type& _Get_container() const { + return _Mycont; + } + // set operations _NODISCARD iterator find(const _Kty& _Val) { return _Find(_Val); @@ -666,16 +672,16 @@ private: const auto _Equivalent = [this](const _Kty& _Lhs, const _Kty& _Rhs) { return !_Compare(_Lhs, _Rhs) && !_Compare(_Rhs, _Lhs); }; - const iterator _End = end(); - _Mycont.erase(_STD unique(begin(), _End, _Equivalent), _End); + const auto _End = _Mycont.end(); + _Mycont.erase(_STD unique(_Mycont.begin(), _End, _Equivalent), _End); } } template void _Restore_invariants_after_insert(const size_type _Old_size) { - const iterator _Begin = begin(); - const iterator _Old_end = _Begin + static_cast(_Old_size); - const iterator _End = end(); + const auto _Begin = _Mycont.begin(); + const auto _Old_end = _Begin + static_cast(_Old_size); + const auto _End = _Mycont.end(); if constexpr (!_Presorted) { _STD sort(_Old_end, _End, _Pass_comp()); @@ -690,15 +696,15 @@ private: } void _Make_invariants_fulfilled() { - const iterator _Begin = begin(); - const iterator _End = end(); + const auto _Begin = _Mycont.begin(); + const auto _End = _Mycont.end(); if (_Begin == _End) { return; } // O(N) if already sorted. - const iterator _Begin_unsorted = _STD is_sorted_until(_Begin, _End, _Pass_comp()); + const auto _Begin_unsorted = _STD is_sorted_until(_Begin, _End, _Pass_comp()); _STD sort(_Begin_unsorted, _End, _Pass_comp()); _STD inplace_merge(_Begin, _Begin_unsorted, _End, _Pass_comp()); @@ -769,8 +775,9 @@ _EXPORT_STD template _Container::size_type erase_if(flat_set<_Kty, _Keylt, _Container>& _Val, _Pred _Predicate) { // clears the container to maintain the invariants when an exception is thrown (N4950 [flat.set.erasure]/5) _Clear_guard> _Guard{_STD addressof(_Val)}; - const auto _Erased_count = _STD _Erase_remove_if(_Val, _STD _Pass_fn(_Predicate)); - _Guard._Target = nullptr; + const auto _Erased_count = + _STD _Erase_remove_if(const_cast<_Container&>(_Val._Get_container()), _STD _Pass_fn(_Predicate)); + _Guard._Target = nullptr; return _Erased_count; } @@ -778,8 +785,9 @@ _EXPORT_STD template _Container::size_type erase_if(flat_multiset<_Kty, _Keylt, _Container>& _Val, _Pred _Predicate) { // clears the container to maintain the invariants when an exception is thrown (N4950 [flat.multiset.erasure]/5) _Clear_guard> _Guard{_STD addressof(_Val)}; - const auto _Erased_count = _STD _Erase_remove_if(_Val, _STD _Pass_fn(_Predicate)); - _Guard._Target = nullptr; + const auto _Erased_count = + _STD _Erase_remove_if(const_cast<_Container&>(_Val._Get_container()), _STD _Pass_fn(_Predicate)); + _Guard._Target = nullptr; return _Erased_count; } From a32d2b7843321b3b7763ed5c46b525558f3e6eec Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Sat, 28 Oct 2023 05:03:10 +0800 Subject: [PATCH 3/7] 3. cleanups etc. --- stl/inc/flat_set | 64 ++++++++++++++++++++---------------------------- 1 file changed, 27 insertions(+), 37 deletions(-) diff --git a/stl/inc/flat_set b/stl/inc/flat_set index 74fb4a0a6b1..479563cbe60 100644 --- a/stl/inc/flat_set +++ b/stl/inc/flat_set @@ -160,7 +160,7 @@ public: {} _Base_flat_set& operator=(const _Base_flat_set& _Other) { - _Clear_guard<_Base_flat_set> _Guard{this}; + _Clear_guard _Guard{_STD addressof(_Mycont)}; _Mycont = _Other._Mycont; _Mycomp = _Other._Mycomp; _Guard._Target = nullptr; @@ -170,8 +170,8 @@ public: is_nothrow_move_assignable_v&& is_nothrow_copy_assignable_v) // strengthened { if (this != _STD addressof(_Other)) { - _Clear_guard<_Base_flat_set> _Guard{this}; - _Clear_guard<_Base_flat_set> _Always_clear{_STD addressof(_Other)}; + _Clear_guard _Guard{_STD addressof(_Mycont)}; + _Clear_guard _Always_clear{_STD addressof(_Other._Mycont)}; _Mycont = _STD move(_Other._Mycont); _Mycomp = _Other._Mycomp; // intentionally copy comparator, see LWG-2227 _Guard._Target = nullptr; @@ -180,7 +180,7 @@ public: } _Deriv& operator=(initializer_list<_Kty> _Ilist) { - _Clear_guard<_Base_flat_set> _Guard{this}; + _Clear_guard _Guard{_STD addressof(_Mycont)}; _Mycont.assign(_Ilist); _Make_invariants_fulfilled(); _Guard._Target = nullptr; @@ -188,17 +188,19 @@ public: } // iterators + // for flat_meow, `const_iterator` is required to be convertible to `iterator` (per N4958 + // [associative.reqmts.general]/6) _NODISCARD iterator begin() noexcept { - return _Mycont.begin(); + return cbegin(); } _NODISCARD const_iterator begin() const noexcept { - return _Mycont.begin(); + return cbegin(); } _NODISCARD iterator end() noexcept { - return _Mycont.end(); + return cend(); } _NODISCARD const_iterator end() const noexcept { - return _Mycont.end(); + return cend(); } _NODISCARD reverse_iterator rbegin() noexcept { @@ -314,12 +316,12 @@ public: _NODISCARD container_type extract() && noexcept( is_nothrow_move_constructible_v) /* strengthened */ { // always clears the container (N4950 [flat.set.modifiers]/14 and [flat.multiset.modifiers]/10) - _Clear_guard<_Base_flat_set> _Always_clear{this}; + _Clear_guard _Always_clear{_STD addressof(_Mycont)}; return _STD move(_Mycont); } void replace(container_type&& _Cont) { _STL_ASSERT(_Is_sorted(_Cont), _Msg_not_sorted); - _Clear_guard<_Base_flat_set> _Guard{this}; + _Clear_guard _Guard{_STD addressof(_Mycont)}; _Mycont = _STD move(_Cont); _Guard._Target = nullptr; } @@ -527,8 +529,8 @@ private: template requires (!_Multi) // flat_set _NODISCARD pair _Emplace(_Ty&& _Val) { - const iterator _Where = lower_bound(_Val); - if (_Where != end() && !_Compare(_Val, *_Where)) { + const const_iterator _Where = lower_bound(_Val); + if (_Where != cend() && !_Compare(_Val, *_Where)) { // *_Where is equivalent to _Val. return pair{_Where, false}; } @@ -595,8 +597,9 @@ private: } if (_Where != _End && !_Compare(_Val, *_Where)) { - // *_Where is equivalent to _Val; convert _Where to iterator type. - return _Mycont.begin() + (_Where - _Begin); + // *_Where is equivalent to _Val. + // for flat_meow, _Where should be convertible to iterator type. + return _Where; } if constexpr (is_same_v, _Kty>) { @@ -626,8 +629,8 @@ private: _STL_INTERNAL_STATIC_ASSERT(_Keylt_transparent || is_same_v<_Ty, _Kty>); if constexpr (!_Multi && is_same_v<_Ty, _Kty>) { - const iterator _Where = lower_bound(_Val); - if (_Where != end() && !_Compare(_Val, *_Where)) { + const const_iterator _Where = lower_bound(_Val); + if (_Where != cend() && !_Compare(_Val, *_Where)) { _Mycont.erase(_Where); return 1; } @@ -641,19 +644,6 @@ private: } } - template - _NODISCARD iterator _Find(const _Ty& _Val) { - _STL_INTERNAL_STATIC_ASSERT(_Keylt_transparent || is_same_v<_Ty, _Kty>); - - const iterator _End = end(); - const iterator _Where = lower_bound(_Val); - if (_Where != _End && !_Compare(_Val, *_Where)) { - return _Where; - } else { - return _End; - } - } - template _NODISCARD const_iterator _Find(const _Ty& _Val) const { _STL_INTERNAL_STATIC_ASSERT(_Keylt_transparent || is_same_v<_Ty, _Kty>); @@ -774,20 +764,20 @@ public: _EXPORT_STD template _Container::size_type erase_if(flat_set<_Kty, _Keylt, _Container>& _Val, _Pred _Predicate) { // clears the container to maintain the invariants when an exception is thrown (N4950 [flat.set.erasure]/5) - _Clear_guard> _Guard{_STD addressof(_Val)}; - const auto _Erased_count = - _STD _Erase_remove_if(const_cast<_Container&>(_Val._Get_container()), _STD _Pass_fn(_Predicate)); - _Guard._Target = nullptr; + _Container& _Cont = const_cast<_Container&>(_Val._Get_container()); + _Clear_guard<_Container> _Guard{_STD addressof(_Cont)}; + const auto _Erased_count = _STD _Erase_remove_if(_Cont, _STD _Pass_fn(_Predicate)); + _Guard._Target = nullptr; return _Erased_count; } _EXPORT_STD template _Container::size_type erase_if(flat_multiset<_Kty, _Keylt, _Container>& _Val, _Pred _Predicate) { // clears the container to maintain the invariants when an exception is thrown (N4950 [flat.multiset.erasure]/5) - _Clear_guard> _Guard{_STD addressof(_Val)}; - const auto _Erased_count = - _STD _Erase_remove_if(const_cast<_Container&>(_Val._Get_container()), _STD _Pass_fn(_Predicate)); - _Guard._Target = nullptr; + _Container& _Cont = const_cast<_Container&>(_Val._Get_container()); + _Clear_guard<_Container> _Guard{_STD addressof(_Cont)}; + const auto _Erased_count = _STD _Erase_remove_if(_Cont, _STD _Pass_fn(_Predicate)); + _Guard._Target = nullptr; return _Erased_count; } From 2f1e8811c9a3b0203191adfa15c60cd1e553b454 Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Sat, 28 Oct 2023 07:46:46 +0800 Subject: [PATCH 4/7] Apply suggestions from code review Co-authored-by: Casey Carter --- stl/inc/flat_set | 6 +----- tests/std/tests/P1222R4_flat_set/test.cpp | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/stl/inc/flat_set b/stl/inc/flat_set index 479563cbe60..bcfc5c2f620 100644 --- a/stl/inc/flat_set +++ b/stl/inc/flat_set @@ -58,8 +58,8 @@ public: using difference_type = _Container::difference_type; using const_iterator = _Container::const_iterator; using iterator = const_iterator; - using reverse_iterator = _STD reverse_iterator; using const_reverse_iterator = _STD reverse_iterator; + using reverse_iterator = const_reverse_iterator; using container_type = _Container; static_assert(random_access_iterator, "The C++ Standard forbids containers without random " @@ -188,8 +188,6 @@ public: } // iterators - // for flat_meow, `const_iterator` is required to be convertible to `iterator` (per N4958 - // [associative.reqmts.general]/6) _NODISCARD iterator begin() noexcept { return cbegin(); } @@ -597,8 +595,6 @@ private: } if (_Where != _End && !_Compare(_Val, *_Where)) { - // *_Where is equivalent to _Val. - // for flat_meow, _Where should be convertible to iterator type. return _Where; } diff --git a/tests/std/tests/P1222R4_flat_set/test.cpp b/tests/std/tests/P1222R4_flat_set/test.cpp index 3586eb40705..803411f78aa 100644 --- a/tests/std/tests/P1222R4_flat_set/test.cpp +++ b/tests/std/tests/P1222R4_flat_set/test.cpp @@ -80,7 +80,7 @@ void assert_set_requirements() { using key_type = T::key_type; using value_type = T::value_type; - static_assert(_Constant_iterator); + static_assert(same_as, const_iterator>); static_assert(is_convertible_v); // additionally: From cda15c2dde584a18a709106487c6adce201513e6 Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Sat, 28 Oct 2023 07:53:41 +0800 Subject: [PATCH 5/7] Apply suggestions from code review 2 --- stl/inc/flat_set | 9 ++++----- tests/std/tests/P1222R4_flat_set/test.cpp | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/stl/inc/flat_set b/stl/inc/flat_set index bcfc5c2f620..99e0833fd03 100644 --- a/stl/inc/flat_set +++ b/stl/inc/flat_set @@ -189,16 +189,16 @@ public: // iterators _NODISCARD iterator begin() noexcept { - return cbegin(); + return _Mycont.cbegin(); } _NODISCARD const_iterator begin() const noexcept { - return cbegin(); + return _Mycont.cbegin(); } _NODISCARD iterator end() noexcept { - return cend(); + return _Mycont.cend(); } _NODISCARD const_iterator end() const noexcept { - return cend(); + return _Mycont.cend(); } _NODISCARD reverse_iterator rbegin() noexcept { @@ -529,7 +529,6 @@ private: _NODISCARD pair _Emplace(_Ty&& _Val) { const const_iterator _Where = lower_bound(_Val); if (_Where != cend() && !_Compare(_Val, *_Where)) { - // *_Where is equivalent to _Val. return pair{_Where, false}; } diff --git a/tests/std/tests/P1222R4_flat_set/test.cpp b/tests/std/tests/P1222R4_flat_set/test.cpp index 803411f78aa..4f423920c67 100644 --- a/tests/std/tests/P1222R4_flat_set/test.cpp +++ b/tests/std/tests/P1222R4_flat_set/test.cpp @@ -85,7 +85,7 @@ void assert_set_requirements() { // additionally: static_assert(is_same_v); - static_assert(_Constant_iterator); + static_assert(same_as, iterator>); static_assert(is_convertible_v); } From 150359d268547874887fe663b121bca5423ab0b0 Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Tue, 31 Oct 2023 05:23:38 +0800 Subject: [PATCH 6/7] review feedback: remove redundant non-const overloads. --- stl/inc/flat_set | 63 +++++------------------------------------------- 1 file changed, 6 insertions(+), 57 deletions(-) diff --git a/stl/inc/flat_set b/stl/inc/flat_set index 99e0833fd03..04012b2eb18 100644 --- a/stl/inc/flat_set +++ b/stl/inc/flat_set @@ -188,28 +188,17 @@ public: } // iterators - _NODISCARD iterator begin() noexcept { - return _Mycont.cbegin(); - } + // NB: The non-const overloads are intentionally removed for brevity. This will not result in behavioral changes. _NODISCARD const_iterator begin() const noexcept { - return _Mycont.cbegin(); - } - _NODISCARD iterator end() noexcept { - return _Mycont.cend(); + return _Mycont.begin(); } _NODISCARD const_iterator end() const noexcept { - return _Mycont.cend(); + return _Mycont.end(); } - _NODISCARD reverse_iterator rbegin() noexcept { - return reverse_iterator(end()); - } _NODISCARD const_reverse_iterator rbegin() const noexcept { return const_reverse_iterator(end()); } - _NODISCARD reverse_iterator rend() noexcept { - return reverse_iterator(begin()); - } _NODISCARD const_reverse_iterator rend() const noexcept { return const_reverse_iterator(begin()); } @@ -324,11 +313,7 @@ public: _Guard._Target = nullptr; } - iterator erase(iterator _Where) - requires (!is_same_v) - { - return _Mycont.erase(_Where); - } + // NB: `erase(iterator)` is identical to `erase(const_iterator)` iterator erase(const_iterator _Where) { return _Mycont.erase(_Where); } @@ -336,8 +321,7 @@ public: return _Erase(_Val); } template <_Different_from<_Kty> _Other> - requires ( - _Keylt_transparent && !is_convertible_v<_Other, iterator> && !is_convertible_v<_Other, const_iterator>) + requires (_Keylt_transparent && !is_convertible_v<_Other, const_iterator>) size_type erase(_Other&& _Val) { return _Erase(_Val); } @@ -366,18 +350,10 @@ public: } // set operations - _NODISCARD iterator find(const _Kty& _Val) { - return _Find(_Val); - } + // NB: The non-const overloads are intentionally removed for brevity. This will not result in behavioral changes. _NODISCARD const_iterator find(const _Kty& _Val) const { return _Find(_Val); } - - template - requires _Keylt_transparent - _NODISCARD iterator find(const _Other& _Val) { - return _Find(_Val); - } template requires _Keylt_transparent _NODISCARD const_iterator find(const _Other& _Val) const { @@ -408,54 +384,27 @@ public: return _STD binary_search(cbegin(), cend(), _Val, _Pass_comp()); } - _NODISCARD iterator lower_bound(const _Kty& _Val) { - return _STD lower_bound(begin(), end(), _Val, _Pass_comp()); - } _NODISCARD const_iterator lower_bound(const _Kty& _Val) const { return _STD lower_bound(cbegin(), cend(), _Val, _Pass_comp()); } - - template - requires _Keylt_transparent - _NODISCARD iterator lower_bound(const _Other& _Val) { - return _STD lower_bound(begin(), end(), _Val, _Pass_comp()); - } template requires _Keylt_transparent _NODISCARD const_iterator lower_bound(const _Other& _Val) const { return _STD lower_bound(cbegin(), cend(), _Val, _Pass_comp()); } - _NODISCARD iterator upper_bound(const _Kty& _Val) { - return _STD upper_bound(begin(), end(), _Val, _Pass_comp()); - } _NODISCARD const_iterator upper_bound(const _Kty& _Val) const { return _STD upper_bound(cbegin(), cend(), _Val, _Pass_comp()); } - - template - requires _Keylt_transparent - _NODISCARD iterator upper_bound(const _Other& _Val) { - return _STD upper_bound(begin(), end(), _Val, _Pass_comp()); - } template requires _Keylt_transparent _NODISCARD const_iterator upper_bound(const _Other& _Val) const { return _STD upper_bound(cbegin(), cend(), _Val, _Pass_comp()); } - _NODISCARD pair equal_range(const _Kty& _Val) { - return _STD equal_range(begin(), end(), _Val, _Pass_comp()); - } _NODISCARD pair equal_range(const _Kty& _Val) const { return _STD equal_range(cbegin(), cend(), _Val, _Pass_comp()); } - - template - requires _Keylt_transparent - _NODISCARD pair equal_range(const _Other& _Val) { - return _STD equal_range(begin(), end(), _Val, _Pass_comp()); - } template requires _Keylt_transparent _NODISCARD pair equal_range(const _Other& _Val) const { From 7189ef90ae96219162e36950154f9823afa9aba3 Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Tue, 31 Oct 2023 05:26:05 +0800 Subject: [PATCH 7/7] review feedback: make _Get_container non-const --- stl/inc/flat_set | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stl/inc/flat_set b/stl/inc/flat_set index 04012b2eb18..99eb74870eb 100644 --- a/stl/inc/flat_set +++ b/stl/inc/flat_set @@ -345,7 +345,7 @@ public: return _Mycomp; } - _NODISCARD const container_type& _Get_container() const { + _NODISCARD container_type& _Get_container_for_erase_if() noexcept { return _Mycont; } @@ -708,7 +708,7 @@ public: _EXPORT_STD template _Container::size_type erase_if(flat_set<_Kty, _Keylt, _Container>& _Val, _Pred _Predicate) { // clears the container to maintain the invariants when an exception is thrown (N4950 [flat.set.erasure]/5) - _Container& _Cont = const_cast<_Container&>(_Val._Get_container()); + _Container& _Cont = _Val._Get_container_for_erase_if(); _Clear_guard<_Container> _Guard{_STD addressof(_Cont)}; const auto _Erased_count = _STD _Erase_remove_if(_Cont, _STD _Pass_fn(_Predicate)); _Guard._Target = nullptr; @@ -718,7 +718,7 @@ _Container::size_type erase_if(flat_set<_Kty, _Keylt, _Container>& _Val, _Pred _ _EXPORT_STD template _Container::size_type erase_if(flat_multiset<_Kty, _Keylt, _Container>& _Val, _Pred _Predicate) { // clears the container to maintain the invariants when an exception is thrown (N4950 [flat.multiset.erasure]/5) - _Container& _Cont = const_cast<_Container&>(_Val._Get_container()); + _Container& _Cont = _Val._Get_container_for_erase_if(); _Clear_guard<_Container> _Guard{_STD addressof(_Cont)}; const auto _Erased_count = _STD _Erase_remove_if(_Cont, _STD _Pass_fn(_Predicate)); _Guard._Target = nullptr;