From c40a251887853dee30c406bbb94522b35195d912 Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Sun, 15 Oct 2023 14:49:03 +0800 Subject: [PATCH 01/22] Make `deque::shrink_to_fit` never relocate elements --- stl/inc/deque | 80 ++++++++++++++----- .../test.cpp | 25 ++++++ .../test.cpp | 2 +- 3 files changed, 88 insertions(+), 19 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 56b0a708568..36b44660d46 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -986,19 +986,58 @@ public: } void shrink_to_fit() { - size_type _Oldcapacity = _Block_size * _Mapsize(); - size_type _Newcapacity = _Oldcapacity / 2; + if (empty()) { + if (_Map() != nullptr) { + _Delete_map(); + } + return; + } + + const auto _First_block_ptr = _Map() + static_cast<_Map_difference_type>(_Myoff() / _Block_size); + const auto _Last_block_ptr = + _Map() + static_cast<_Map_difference_type>((_Myoff() + _Mysize() - 1) / _Block_size + 1); + const auto _Used_block_count = static_cast(_Last_block_ptr - _First_block_ptr); - if (_Newcapacity < _Block_size * _Minimum_map_size) { - _Newcapacity = _Block_size * _Minimum_map_size; + // deallocate unused blocks + for (auto _Block_it = _Map(); _Block_it != _First_block_ptr; ++_Block_it) { + if (*_Block_it != nullptr) { + _Getal().deallocate(*_Block_it, _Block_size); + *_Block_it = nullptr; + } + } + for (auto _Block_it = _Last_block_ptr; _Block_it != _Map() + _Map_distance(); ++_Block_it) { + if (*_Block_it != nullptr) { + _Getal().deallocate(*_Block_it, _Block_size); + *_Block_it = nullptr; + } } - if ((empty() && _Mapsize() > 0) - || (!empty() && size() <= _Newcapacity && _Newcapacity < _Oldcapacity)) { // worth shrinking, do it - deque _Tmp( - _STD make_move_iterator(_Unchecked_begin()), _STD make_move_iterator(_Unchecked_end()), _Getal()); - swap(_Tmp); + size_type _New_block_count = 1; // should be power of 2 + for (; _New_block_count < _Used_block_count || _New_block_count < _Minimum_map_size; _New_block_count *= 2) + ; + if (_New_block_count >= _Mapsize()) { + return; } + + // worth shrinking the internal map, do it + + _Alpty _Almap(_Getal()); + const auto _New_map = _Almap.allocate(_New_block_count); + + _Orphan_all(); // the map can be shrinked, invalidate all iterators + + const auto _Copy_end = _STD _Uninitialized_copy_unchecked( + _First_block_ptr, _Last_block_ptr, _New_map); // transfer the ownership of blocks to pointers in the new map + _STD _Uninitialized_value_construct_n_unchecked1( + _Copy_end, _New_block_count - _Used_block_count); // zero out the rest of the new map + _STD fill( + _First_block_ptr, _Last_block_ptr, nullptr); // zero out the old map because the ownership is transferred + + _Delete_map(); + + _Map() = _New_map; + _Mapsize() = _New_block_count; + _Myoff() %= _Block_size; } _NODISCARD const_reference operator[](size_type _Pos) const noexcept /* strengthened */ { @@ -1591,23 +1630,28 @@ private: _Mapsize() += _Count; } + void _Delete_map() noexcept { + _Alpty _Almap(_Getal()); + + for (auto _Block = _Map_distance(); _Block > 0;) { // free storage for a block and destroy pointer + if (_Map()[--_Block]) { // free block + _Getal().deallocate(_Map()[_Block], _Block_size); + } + _STD _Destroy_in_place(_Map()[_Block]); // destroy pointer to block + } + + _Almap.deallocate(_Map(), _Mapsize()); // free storage for map + } + void _Tidy() noexcept { // free all storage _Orphan_all(); - _Alpty _Almap(_Getal()); while (!empty()) { pop_back(); } if (_Map() != nullptr) { - for (auto _Block = _Map_distance(); _Block > 0;) { // free storage for a block and destroy pointer - if (_Map()[--_Block]) { // free block - _Getal().deallocate(_Map()[_Block], _Block_size); - } - _Destroy_in_place(_Map()[_Block]); // destroy pointer to block - } - - _Almap.deallocate(_Map(), _Mapsize()); // free storage for map + _Delete_map(); } _Mapsize() = 0; diff --git a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp index 72ff7bd48c4..235f583ec57 100644 --- a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp +++ b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp @@ -259,6 +259,29 @@ void test_exception_safety_for_throwing_movable() { assert(d == d_orig); } +// Also test GH-4072: shrink_to_fit() should have move_if_noexcept() logic bug +void test_gh_4072() { + constexpr int removed_count = 768; + + deque d; + for (int i = 0; i < 1729; ++i) { + d.emplace_back(i); + } + + for (int i = 0; i < removed_count; ++i) { + d.pop_front(); + d.pop_back(); + } + + deque d2; + for (int i = removed_count; i < 1729 - removed_count; ++i) { + d2.emplace_back(i); + } + + d.shrink_to_fit(); // additionally ensures that no constructor or assignment operator of the element type is called + assert(d == d2); +} + int main() { test_push_back_pop_front(); @@ -266,4 +289,6 @@ int main() { test_exception_safety_for_nonswappable_movable(); test_exception_safety_for_throwing_movable(); + + test_gh_4072(); } diff --git a/tests/std/tests/VSO_0000000_allocator_propagation/test.cpp b/tests/std/tests/VSO_0000000_allocator_propagation/test.cpp index c07d9a65811..d0286de237a 100644 --- a/tests/std/tests/VSO_0000000_allocator_propagation/test.cpp +++ b/tests/std/tests/VSO_0000000_allocator_propagation/test.cpp @@ -506,7 +506,7 @@ void test_deque_shrink_to_fit_per_alloc() { } } -void test_deque_shrink_to_fit() { // MSVC STL's deque::shrink_to_fit relies on swap +void test_deque_shrink_to_fit() { // regression test: MSVC STL's deque::shrink_to_fit used to rely on swap test_deque_shrink_to_fit_per_alloc>(); test_deque_shrink_to_fit_per_alloc>(); test_deque_shrink_to_fit_per_alloc>(); From ba4c9b576f1b63b63049dfd9bf7b9625aa5bd12f Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Sun, 15 Oct 2023 15:20:33 +0800 Subject: [PATCH 02/22] Fix deletion of the map --- stl/inc/deque | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 36b44660d46..46acb0084f2 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -988,7 +988,7 @@ public: void shrink_to_fit() { if (empty()) { if (_Map() != nullptr) { - _Delete_map(); + _Reset_map(); } return; } @@ -1033,7 +1033,7 @@ public: _STD fill( _First_block_ptr, _Last_block_ptr, nullptr); // zero out the old map because the ownership is transferred - _Delete_map(); + _Reset_map(); _Map() = _New_map; _Mapsize() = _New_block_count; @@ -1630,7 +1630,9 @@ private: _Mapsize() += _Count; } - void _Delete_map() noexcept { + void _Reset_map() noexcept { + // pre: each block pointer is either null or pointing to a block without constructed elements + _Alpty _Almap(_Getal()); for (auto _Block = _Map_distance(); _Block > 0;) { // free storage for a block and destroy pointer @@ -1641,6 +1643,9 @@ private: } _Almap.deallocate(_Map(), _Mapsize()); // free storage for map + + _Map() = nullptr; + _Mapsize() = 0; } void _Tidy() noexcept { // free all storage @@ -1651,7 +1656,7 @@ private: } if (_Map() != nullptr) { - _Delete_map(); + _Reset_map(); } _Mapsize() = 0; From 01d1101a153b56473c69e2ac513173ea3669d41a Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Sun, 15 Oct 2023 21:34:13 +0800 Subject: [PATCH 03/22] Address @achabense's review comments Properly handle the internal circular buffer! --- stl/inc/deque | 50 +++++++++++-------- .../test.cpp | 8 +++ 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 46acb0084f2..3b12e232799 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -993,27 +993,33 @@ public: return; } - const auto _First_block_ptr = _Map() + static_cast<_Map_difference_type>(_Myoff() / _Block_size); - const auto _Last_block_ptr = - _Map() + static_cast<_Map_difference_type>((_Myoff() + _Mysize() - 1) / _Block_size + 1); - const auto _Used_block_count = static_cast(_Last_block_ptr - _First_block_ptr); + const auto _First_block_idx = static_cast<_Map_difference_type>(_Myoff() / _Block_size); + const auto _Last_block_idx = + static_cast<_Map_difference_type>(((_Myoff() + _Mysize() - 1) / _Block_size + 1) & (_Mapsize() - 1)); // deallocate unused blocks - for (auto _Block_it = _Map(); _Block_it != _First_block_ptr; ++_Block_it) { - if (*_Block_it != nullptr) { - _Getal().deallocate(*_Block_it, _Block_size); - *_Block_it = nullptr; + + for (auto _Block_idx = _Map_difference_type{0}; _Block_idx != _First_block_idx; ++_Block_idx) { + auto& _Block_ptr = _Map()[static_cast<_Map_difference_type>(_Block_idx & (_Mapsize() - 1))]; + if (_Block_ptr != nullptr) { + _Getal().deallocate(_Block_ptr, _Block_size); + _Block_ptr = nullptr; } } - for (auto _Block_it = _Last_block_ptr; _Block_it != _Map() + _Map_distance(); ++_Block_it) { - if (*_Block_it != nullptr) { - _Getal().deallocate(*_Block_it, _Block_size); - *_Block_it = nullptr; + + for (auto _Block_idx = _Last_block_idx; _Block_idx != _Map_distance(); ++_Block_idx) { + auto& _Block_ptr = _Map()[static_cast<_Map_difference_type>(_Block_idx & (_Mapsize() - 1))]; + if (_Block_ptr != nullptr) { + _Getal().deallocate(_Block_ptr, _Block_size); + _Block_ptr = nullptr; } } - size_type _New_block_count = 1; // should be power of 2 - for (; _New_block_count < _Used_block_count || _New_block_count < _Minimum_map_size; _New_block_count *= 2) + const auto _Used_block_count = + static_cast((_Myoff() + _Mysize() - 1) / _Block_size + 1 - _Myoff() / _Block_size); + + size_type _New_block_count = _Minimum_map_size; // should be power of 2 + for (; _New_block_count < _Used_block_count; _New_block_count *= 2) ; if (_New_block_count >= _Mapsize()) { return; @@ -1026,12 +1032,16 @@ public: _Orphan_all(); // the map can be shrinked, invalidate all iterators - const auto _Copy_end = _STD _Uninitialized_copy_unchecked( - _First_block_ptr, _Last_block_ptr, _New_map); // transfer the ownership of blocks to pointers in the new map - _STD _Uninitialized_value_construct_n_unchecked1( - _Copy_end, _New_block_count - _Used_block_count); // zero out the rest of the new map - _STD fill( - _First_block_ptr, _Last_block_ptr, nullptr); // zero out the old map because the ownership is transferred + _Map_difference_type _New_block_idx = 0; + for (_Map_difference_type _Block_idx = _First_block_idx; _Block_idx != _Last_block_idx; + ++_Block_idx, ++_New_block_idx) { + // transfer the ownership of blocks to pointers in the new map + _STD _Construct_in_place(_New_map[_New_block_idx], + _STD exchange(_Map()[static_cast<_Map_difference_type>(_Block_idx & (_Mapsize() - 1))], nullptr)); + } + for (; _New_block_idx != static_cast<_Map_difference_type>(_New_block_count); ++_New_block_idx) { + _STD _Construct_in_place(_New_map[_New_block_idx]); // zero out the rest of the new map + } _Reset_map(); diff --git a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp index 235f583ec57..9eac130fcc3 100644 --- a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp +++ b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp @@ -280,6 +280,14 @@ void test_gh_4072() { d.shrink_to_fit(); // additionally ensures that no constructor or assignment operator of the element type is called assert(d == d2); + + // ensure that the circular buffer is correctly handled + { + std::deque deq(128); + deq.pop_back(); + deq.push_front(0); + deq.shrink_to_fit(); + } } int main() { From a00c1412e0439ae64d97cc6f7fdc23e0add64631 Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Sun, 15 Oct 2023 23:11:41 +0800 Subject: [PATCH 04/22] Fix the block indices of the circular buffer --- stl/inc/deque | 20 ++++++------------- .../test.cpp | 14 +++++++++++-- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 3b12e232799..95a3bde4edc 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -999,16 +999,9 @@ public: // deallocate unused blocks - for (auto _Block_idx = _Map_difference_type{0}; _Block_idx != _First_block_idx; ++_Block_idx) { - auto& _Block_ptr = _Map()[static_cast<_Map_difference_type>(_Block_idx & (_Mapsize() - 1))]; - if (_Block_ptr != nullptr) { - _Getal().deallocate(_Block_ptr, _Block_size); - _Block_ptr = nullptr; - } - } - - for (auto _Block_idx = _Last_block_idx; _Block_idx != _Map_distance(); ++_Block_idx) { - auto& _Block_ptr = _Map()[static_cast<_Map_difference_type>(_Block_idx & (_Mapsize() - 1))]; + for (auto _Block_idx = _Last_block_idx; _Block_idx != _First_block_idx; + _Block_idx = static_cast<_Map_difference_type>((_Block_idx + 1) & (_Mapsize() - 1))) { + auto& _Block_ptr = _Map()[_Block_idx]; if (_Block_ptr != nullptr) { _Getal().deallocate(_Block_ptr, _Block_size); _Block_ptr = nullptr; @@ -1033,11 +1026,10 @@ public: _Orphan_all(); // the map can be shrinked, invalidate all iterators _Map_difference_type _New_block_idx = 0; - for (_Map_difference_type _Block_idx = _First_block_idx; _Block_idx != _Last_block_idx; - ++_Block_idx, ++_New_block_idx) { + for (auto _Block_idx = _First_block_idx; _Block_idx != _Last_block_idx; + _Block_idx = static_cast<_Map_difference_type>((_Block_idx + 1) & (_Mapsize() - 1)), ++_New_block_idx) { // transfer the ownership of blocks to pointers in the new map - _STD _Construct_in_place(_New_map[_New_block_idx], - _STD exchange(_Map()[static_cast<_Map_difference_type>(_Block_idx & (_Mapsize() - 1))], nullptr)); + _STD _Construct_in_place(_New_map[_New_block_idx], _STD exchange(_Map()[_Block_idx], nullptr)); } for (; _New_block_idx != static_cast<_Map_difference_type>(_New_block_count); ++_New_block_idx) { _STD _Construct_in_place(_New_map[_New_block_idx]); // zero out the rest of the new map diff --git a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp index 9eac130fcc3..55c5b223efe 100644 --- a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp +++ b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp @@ -283,9 +283,19 @@ void test_gh_4072() { // ensure that the circular buffer is correctly handled { - std::deque deq(128); + deque deq(128); deq.pop_back(); - deq.push_front(0); + deq.emplace_front(0); + deq.shrink_to_fit(); + } + { + deque deq(128); + for (int i = 0; i < 120; i++) { + deq.pop_back(); + } + for (int i = 0; i < 5; i++) { + deq.emplace_front(0); + } deq.shrink_to_fit(); } } From 43f602fc98e6edd5ddd2366b5f1a76e45fc2ee33 Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Sun, 15 Oct 2023 23:49:40 +0800 Subject: [PATCH 05/22] Reduce nullity check Co-authored-by: achabense <60953653+achabense@users.noreply.github.com> --- stl/inc/deque | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 95a3bde4edc..d3b968f0aed 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -998,7 +998,6 @@ public: static_cast<_Map_difference_type>(((_Myoff() + _Mysize() - 1) / _Block_size + 1) & (_Mapsize() - 1)); // deallocate unused blocks - for (auto _Block_idx = _Last_block_idx; _Block_idx != _First_block_idx; _Block_idx = static_cast<_Map_difference_type>((_Block_idx + 1) & (_Mapsize() - 1))) { auto& _Block_ptr = _Map()[_Block_idx]; @@ -1029,13 +1028,17 @@ public: for (auto _Block_idx = _First_block_idx; _Block_idx != _Last_block_idx; _Block_idx = static_cast<_Map_difference_type>((_Block_idx + 1) & (_Mapsize() - 1)), ++_New_block_idx) { // transfer the ownership of blocks to pointers in the new map - _STD _Construct_in_place(_New_map[_New_block_idx], _STD exchange(_Map()[_Block_idx], nullptr)); - } - for (; _New_block_idx != static_cast<_Map_difference_type>(_New_block_count); ++_New_block_idx) { - _STD _Construct_in_place(_New_map[_New_block_idx]); // zero out the rest of the new map + _STD _Construct_in_place(_New_map[_New_block_idx], _Map()[_Block_idx]); } + // zero out the rest of the new map + _STD _Uninitialized_value_construct_n_unchecked1( + _New_map + _New_block_idx, _New_block_count - static_cast(_New_block_idx)); - _Reset_map(); + for (auto _Block = _Map_distance(); _Block > 0;) { + --_Block; + _STD _Destroy_in_place(_Map()[_Block]); // destroy pointer to block + } + _Almap.deallocate(_Map(), _Mapsize()); // free storage for map _Map() = _New_map; _Mapsize() = _New_block_count; From e22c30f493d8b9208c44dbfd74ba2f5ccde365fb Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Wed, 18 Oct 2023 01:34:27 +0800 Subject: [PATCH 06/22] Improve comments and variable names --- stl/inc/deque | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index d3b968f0aed..e6966a790fa 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -993,12 +993,12 @@ public: return; } - const auto _First_block_idx = static_cast<_Map_difference_type>(_Myoff() / _Block_size); - const auto _Last_block_idx = + const auto _First_used_block_idx = static_cast<_Map_difference_type>(_Myoff() / _Block_size); + const auto _First_unused_block_idx = static_cast<_Map_difference_type>(((_Myoff() + _Mysize() - 1) / _Block_size + 1) & (_Mapsize() - 1)); - // deallocate unused blocks - for (auto _Block_idx = _Last_block_idx; _Block_idx != _First_block_idx; + // deallocate unused blocks, traversing over the circular buffer until the first used block index + for (auto _Block_idx = _First_unused_block_idx; _Block_idx != _First_used_block_idx; _Block_idx = static_cast<_Map_difference_type>((_Block_idx + 1) & (_Mapsize() - 1))) { auto& _Block_ptr = _Map()[_Block_idx]; if (_Block_ptr != nullptr) { @@ -1022,10 +1022,10 @@ public: _Alpty _Almap(_Getal()); const auto _New_map = _Almap.allocate(_New_block_count); - _Orphan_all(); // the map can be shrinked, invalidate all iterators + _Orphan_all(); // the map can be shrunk, invalidate all iterators _Map_difference_type _New_block_idx = 0; - for (auto _Block_idx = _First_block_idx; _Block_idx != _Last_block_idx; + for (auto _Block_idx = _First_used_block_idx; _Block_idx != _First_unused_block_idx; _Block_idx = static_cast<_Map_difference_type>((_Block_idx + 1) & (_Mapsize() - 1)), ++_New_block_idx) { // transfer the ownership of blocks to pointers in the new map _STD _Construct_in_place(_New_map[_New_block_idx], _Map()[_Block_idx]); From 24f3a0f6785c3f944e502a2fcd9804536716135e Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 21:42:21 -0800 Subject: [PATCH 07/22] Add braces, `for` => `while`. --- stl/inc/deque | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 492a4b0557a..d6b29ff376e 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -1014,8 +1014,11 @@ public: static_cast((_Myoff() + _Mysize() - 1) / _Block_size + 1 - _Myoff() / _Block_size); size_type _New_block_count = _Minimum_map_size; // should be power of 2 - for (; _New_block_count < _Used_block_count; _New_block_count *= 2) - ; + + while (_New_block_count < _Used_block_count) { + _New_block_count *= 2; + } + if (_New_block_count >= _Mapsize()) { return; } From 6d5e16853c27980cdb9a8dbd443fd418e425fd21 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 22:01:40 -0800 Subject: [PATCH 08/22] Style: Add parens around division. --- stl/inc/deque | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index d6b29ff376e..d1cc790aeee 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -998,7 +998,7 @@ public: const auto _First_used_block_idx = static_cast<_Map_difference_type>(_Myoff() / _Block_size); const auto _First_unused_block_idx = - static_cast<_Map_difference_type>(((_Myoff() + _Mysize() - 1) / _Block_size + 1) & (_Mapsize() - 1)); + static_cast<_Map_difference_type>((((_Myoff() + _Mysize() - 1) / _Block_size) + 1) & (_Mapsize() - 1)); // deallocate unused blocks, traversing over the circular buffer until the first used block index for (auto _Block_idx = _First_unused_block_idx; _Block_idx != _First_used_block_idx; @@ -1011,7 +1011,7 @@ public: } const auto _Used_block_count = - static_cast((_Myoff() + _Mysize() - 1) / _Block_size + 1 - _Myoff() / _Block_size); + static_cast(((_Myoff() + _Mysize() - 1) / _Block_size) + 1 - (_Myoff() / _Block_size)); size_type _New_block_count = _Minimum_map_size; // should be power of 2 From 51aab9c63a462f3d76b64971aecc16639af77220 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 22:39:20 -0800 Subject: [PATCH 09/22] Use `size_type` more, wait until the last moment to `static_cast<_Map_difference_type>`. --- stl/inc/deque | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index d1cc790aeee..fc68844ad9f 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -996,14 +996,14 @@ public: return; } - const auto _First_used_block_idx = static_cast<_Map_difference_type>(_Myoff() / _Block_size); + const auto _First_used_block_idx = static_cast(_Myoff() / _Block_size); const auto _First_unused_block_idx = - static_cast<_Map_difference_type>((((_Myoff() + _Mysize() - 1) / _Block_size) + 1) & (_Mapsize() - 1)); + static_cast((((_Myoff() + _Mysize() - 1) / _Block_size) + 1) & (_Mapsize() - 1)); // deallocate unused blocks, traversing over the circular buffer until the first used block index for (auto _Block_idx = _First_unused_block_idx; _Block_idx != _First_used_block_idx; - _Block_idx = static_cast<_Map_difference_type>((_Block_idx + 1) & (_Mapsize() - 1))) { - auto& _Block_ptr = _Map()[_Block_idx]; + _Block_idx = static_cast((_Block_idx + 1) & (_Mapsize() - 1))) { + auto& _Block_ptr = _Map()[static_cast<_Map_difference_type>(_Block_idx)]; if (_Block_ptr != nullptr) { _Getal().deallocate(_Block_ptr, _Block_size); _Block_ptr = nullptr; @@ -1032,9 +1032,9 @@ public: _Map_difference_type _New_block_idx = 0; for (auto _Block_idx = _First_used_block_idx; _Block_idx != _First_unused_block_idx; - _Block_idx = static_cast<_Map_difference_type>((_Block_idx + 1) & (_Mapsize() - 1)), ++_New_block_idx) { + _Block_idx = static_cast((_Block_idx + 1) & (_Mapsize() - 1)), ++_New_block_idx) { // transfer the ownership of blocks to pointers in the new map - _STD _Construct_in_place(_New_map[_New_block_idx], _Map()[_Block_idx]); + _STD _Construct_in_place(_New_map[_New_block_idx], _Map()[static_cast<_Map_difference_type>(_Block_idx)]); } // zero out the rest of the new map _STD _Uninitialized_value_construct_n_unchecked1( From 104bb93421dbfaccf09af5e4350efad5eee485bb Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 22:41:41 -0800 Subject: [PATCH 10/22] Reuse `_First_used_block_idx`. --- stl/inc/deque | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/deque b/stl/inc/deque index fc68844ad9f..8bde77d266b 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -1011,7 +1011,7 @@ public: } const auto _Used_block_count = - static_cast(((_Myoff() + _Mysize() - 1) / _Block_size) + 1 - (_Myoff() / _Block_size)); + static_cast(((_Myoff() + _Mysize() - 1) / _Block_size) + 1 - _First_used_block_idx); size_type _New_block_count = _Minimum_map_size; // should be power of 2 From b6e2732f7c00f689218b8c22631d41bb0cf06cfa Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 22:44:33 -0800 Subject: [PATCH 11/22] Extract `(_Mapsize() - 1)` as the `_Mask`. --- stl/inc/deque | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 8bde77d266b..8f15919fcf7 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -996,13 +996,15 @@ public: return; } + const auto _Mask = static_cast(_Mapsize() - 1); + const auto _First_used_block_idx = static_cast(_Myoff() / _Block_size); const auto _First_unused_block_idx = - static_cast((((_Myoff() + _Mysize() - 1) / _Block_size) + 1) & (_Mapsize() - 1)); + static_cast((((_Myoff() + _Mysize() - 1) / _Block_size) + 1) & _Mask); // deallocate unused blocks, traversing over the circular buffer until the first used block index for (auto _Block_idx = _First_unused_block_idx; _Block_idx != _First_used_block_idx; - _Block_idx = static_cast((_Block_idx + 1) & (_Mapsize() - 1))) { + _Block_idx = static_cast((_Block_idx + 1) & _Mask)) { auto& _Block_ptr = _Map()[static_cast<_Map_difference_type>(_Block_idx)]; if (_Block_ptr != nullptr) { _Getal().deallocate(_Block_ptr, _Block_size); @@ -1032,7 +1034,7 @@ public: _Map_difference_type _New_block_idx = 0; for (auto _Block_idx = _First_used_block_idx; _Block_idx != _First_unused_block_idx; - _Block_idx = static_cast((_Block_idx + 1) & (_Mapsize() - 1)), ++_New_block_idx) { + _Block_idx = static_cast((_Block_idx + 1) & _Mask), ++_New_block_idx) { // transfer the ownership of blocks to pointers in the new map _STD _Construct_in_place(_New_map[_New_block_idx], _Map()[static_cast<_Map_difference_type>(_Block_idx)]); } From 1d0584d5156836f39ca290b484f93b2e875104fc Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 23:02:33 -0800 Subject: [PATCH 12/22] Extract `_Unmasked_first_unused_block_idx`. --- stl/inc/deque | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 8f15919fcf7..e72c5061555 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -999,8 +999,9 @@ public: const auto _Mask = static_cast(_Mapsize() - 1); const auto _First_used_block_idx = static_cast(_Myoff() / _Block_size); - const auto _First_unused_block_idx = - static_cast((((_Myoff() + _Mysize() - 1) / _Block_size) + 1) & _Mask); + const auto _Unmasked_first_unused_block_idx = + static_cast(((_Myoff() + _Mysize() - 1) / _Block_size) + 1); + const auto _First_unused_block_idx = static_cast(_Unmasked_first_unused_block_idx & _Mask); // deallocate unused blocks, traversing over the circular buffer until the first used block index for (auto _Block_idx = _First_unused_block_idx; _Block_idx != _First_used_block_idx; @@ -1012,8 +1013,7 @@ public: } } - const auto _Used_block_count = - static_cast(((_Myoff() + _Mysize() - 1) / _Block_size) + 1 - _First_used_block_idx); + const auto _Used_block_count = static_cast(_Unmasked_first_unused_block_idx - _First_used_block_idx); size_type _New_block_count = _Minimum_map_size; // should be power of 2 From 881672e196a459d3341bcdd833917608e686f28e Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 23:49:53 -0800 Subject: [PATCH 13/22] Use `_Used_block_count` instead of the final value of `_New_block_idx`. --- stl/inc/deque | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/stl/inc/deque b/stl/inc/deque index e72c5061555..69bd4ca4134 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -1038,9 +1038,10 @@ public: // transfer the ownership of blocks to pointers in the new map _STD _Construct_in_place(_New_map[_New_block_idx], _Map()[static_cast<_Map_difference_type>(_Block_idx)]); } + // zero out the rest of the new map _STD _Uninitialized_value_construct_n_unchecked1( - _New_map + _New_block_idx, _New_block_count - static_cast(_New_block_idx)); + _New_map + static_cast<_Map_difference_type>(_Used_block_count), _New_block_count - _Used_block_count); for (auto _Block = _Map_distance(); _Block > 0;) { --_Block; From 6d57266e4e2ec264f5deb65506e13876cf8b0bfc Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 26 Jan 2024 00:06:54 -0800 Subject: [PATCH 14/22] Use a single loop-scoped variable in `[0, _Used_block_count)` to transfer ownership. --- stl/inc/deque | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 69bd4ca4134..0377ea5c77e 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -1032,11 +1032,11 @@ public: _Orphan_all(); // the map can be shrunk, invalidate all iterators - _Map_difference_type _New_block_idx = 0; - for (auto _Block_idx = _First_used_block_idx; _Block_idx != _First_unused_block_idx; - _Block_idx = static_cast((_Block_idx + 1) & _Mask), ++_New_block_idx) { - // transfer the ownership of blocks to pointers in the new map - _STD _Construct_in_place(_New_map[_New_block_idx], _Map()[static_cast<_Map_difference_type>(_Block_idx)]); + // transfer the ownership of blocks to pointers in the new map + for (size_type _Block = 0; _Block != _Used_block_count; ++_Block) { + const auto _New_block_idx = static_cast<_Map_difference_type>(_Block); + const auto _Old_block_idx = static_cast<_Map_difference_type>((_First_used_block_idx + _Block) & _Mask); + _STD _Construct_in_place(_New_map[_New_block_idx], _Map()[_Old_block_idx]); } // zero out the rest of the new map From 6a18ee3259c1616fcc1fb1f6fc2da61100bb10b8 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 26 Jan 2024 00:26:27 -0800 Subject: [PATCH 15/22] Comment: "zero out" => "null out" --- stl/inc/deque | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/deque b/stl/inc/deque index 0377ea5c77e..52b171fdacb 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -1039,7 +1039,7 @@ public: _STD _Construct_in_place(_New_map[_New_block_idx], _Map()[_Old_block_idx]); } - // zero out the rest of the new map + // null out the rest of the new map _STD _Uninitialized_value_construct_n_unchecked1( _New_map + static_cast<_Map_difference_type>(_Used_block_count), _New_block_count - _Used_block_count); From 87fee6df37604f9fbc4bb19bf7089050c5bd8c85 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 26 Jan 2024 00:31:46 -0800 Subject: [PATCH 16/22] In `_Reset_map()`, rebind `_Almap` right before we need it. --- stl/inc/deque | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 52b171fdacb..328d8b5690c 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -1644,8 +1644,6 @@ private: void _Reset_map() noexcept { // pre: each block pointer is either null or pointing to a block without constructed elements - _Alpty _Almap(_Getal()); - for (auto _Block = _Map_distance(); _Block > 0;) { // free storage for a block and destroy pointer if (_Map()[--_Block]) { // free block _Getal().deallocate(_Map()[_Block], _Block_size); @@ -1653,6 +1651,7 @@ private: _STD _Destroy_in_place(_Map()[_Block]); // destroy pointer to block } + _Alpty _Almap(_Getal()); _Almap.deallocate(_Map(), _Mapsize()); // free storage for map _Map() = nullptr; From 85833f0f64724e66b75a27e803c0438046abf5be Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 26 Jan 2024 00:35:24 -0800 Subject: [PATCH 17/22] In `_Reset_map()`, extract `--_Block;` and `auto& _Block_ptr`. --- stl/inc/deque | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index 328d8b5690c..fcc1ed048c2 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -1645,10 +1645,12 @@ private: // pre: each block pointer is either null or pointing to a block without constructed elements for (auto _Block = _Map_distance(); _Block > 0;) { // free storage for a block and destroy pointer - if (_Map()[--_Block]) { // free block - _Getal().deallocate(_Map()[_Block], _Block_size); + --_Block; + auto& _Block_ptr = _Map()[_Block]; + if (_Block_ptr) { // free block + _Getal().deallocate(_Block_ptr, _Block_size); } - _STD _Destroy_in_place(_Map()[_Block]); // destroy pointer to block + _STD _Destroy_in_place(_Block_ptr); // destroy pointer to block } _Alpty _Almap(_Getal()); From 72eb93fc43ccaf43977989bc8df748c8dd003f20 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 26 Jan 2024 00:50:25 -0800 Subject: [PATCH 18/22] Replace redundant code in `_Tidy()` with `_STL_INTERNAL_CHECK(_Mapsize() == 0)`. --- stl/inc/deque | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/stl/inc/deque b/stl/inc/deque index fcc1ed048c2..30c528b8717 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -1671,8 +1671,7 @@ private: _Reset_map(); } - _Mapsize() = 0; - _Map() = nullptr; + _STL_INTERNAL_CHECK(_Mapsize() == 0); // null map should always be paired with zero mapsize } #if _ITERATOR_DEBUG_LEVEL == 2 From 6e4b9254925a15596c67e9ffe27d68ae6d9c258d Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 26 Jan 2024 01:09:44 -0800 Subject: [PATCH 19/22] Add comments to explain calculations. --- stl/inc/deque | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/stl/inc/deque b/stl/inc/deque index 30c528b8717..ba0794eb243 100644 --- a/stl/inc/deque +++ b/stl/inc/deque @@ -999,8 +999,13 @@ public: const auto _Mask = static_cast(_Mapsize() - 1); const auto _First_used_block_idx = static_cast(_Myoff() / _Block_size); + + // (_Myoff() + _Mysize() - 1) is for the last element, i.e. the back() of the deque. + // Divide by _Block_size to get the unmasked index of the last used block. + // Add 1 to get the unmasked index of the first unused block. const auto _Unmasked_first_unused_block_idx = static_cast(((_Myoff() + _Mysize() - 1) / _Block_size) + 1); + const auto _First_unused_block_idx = static_cast(_Unmasked_first_unused_block_idx & _Mask); // deallocate unused blocks, traversing over the circular buffer until the first used block index @@ -1051,7 +1056,7 @@ public: _Map() = _New_map; _Mapsize() = _New_block_count; - _Myoff() %= _Block_size; + _Myoff() %= _Block_size; // the first element is within block index 0 of the new map } _NODISCARD const_reference operator[](size_type _Pos) const noexcept /* strengthened */ { From 9ac04c979fd19723f1f853a57b2fb434780d3cc3 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 26 Jan 2024 01:16:22 -0800 Subject: [PATCH 20/22] Update bug title. --- tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp index d235c3db5d5..03fee284941 100644 --- a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp +++ b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp @@ -259,7 +259,7 @@ void test_exception_safety_for_throwing_movable() { assert(d == d_orig); } -// Also test GH-4072: shrink_to_fit() should have move_if_noexcept() logic bug +// Also test GH-4072: : shrink_to_fit() should follow the Standard void test_gh_4072() { constexpr int removed_count = 768; From 66d308aff7db3b2dc3d7ae508a01bd3b76ff28ae Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 26 Jan 2024 01:23:17 -0800 Subject: [PATCH 21/22] Add a scope to the test; adjust comment to fit in 120 columns. --- .../test.cpp | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp index 03fee284941..c313beb4a3e 100644 --- a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp +++ b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp @@ -261,25 +261,27 @@ void test_exception_safety_for_throwing_movable() { // Also test GH-4072: : shrink_to_fit() should follow the Standard void test_gh_4072() { - constexpr int removed_count = 768; + { + constexpr int removed_count = 768; - deque d; - for (int i = 0; i < 1729; ++i) { - d.emplace_back(i); - } + deque d; + for (int i = 0; i < 1729; ++i) { + d.emplace_back(i); + } - for (int i = 0; i < removed_count; ++i) { - d.pop_front(); - d.pop_back(); - } + for (int i = 0; i < removed_count; ++i) { + d.pop_front(); + d.pop_back(); + } - deque d2; - for (int i = removed_count; i < 1729 - removed_count; ++i) { - d2.emplace_back(i); - } + deque d2; + for (int i = removed_count; i < 1729 - removed_count; ++i) { + d2.emplace_back(i); + } - d.shrink_to_fit(); // additionally ensures that no constructor or assignment operator of the element type is called - assert(d == d2); + d.shrink_to_fit(); // ensures that no constructor or assignment operator of the element type is called + assert(d == d2); + } // ensure that the circular buffer is correctly handled { From 294ff2da0f3a3df32dbac44fc261429bbae59bfe Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 26 Jan 2024 01:24:34 -0800 Subject: [PATCH 22/22] `i++` => `++i`. --- .../std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp index c313beb4a3e..fd362b915da 100644 --- a/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp +++ b/tests/std/tests/Dev10_860421_deque_push_back_pop_front/test.cpp @@ -292,10 +292,10 @@ void test_gh_4072() { } { deque deq(128); - for (int i = 0; i < 120; i++) { + for (int i = 0; i < 120; ++i) { deq.pop_back(); } - for (int i = 0; i < 5; i++) { + for (int i = 0; i < 5; ++i) { deq.emplace_front(0); } deq.shrink_to_fit();