`<deque>`: Properly destroy (fancy) pointers to blocks in the internal map (#2775)

Co-authored-by: Michael Schellenberger Costa <mschellenbergercosta@gmail.com>
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>

stl/inc/deque

@@ -693,7 +693,7 @@ private:
 
     void _Construct_n(size_type _Count, const _Ty& _Val) { // construct from _Count * _Val
         _Tidy_guard<deque> _Guard{this};
-        for (; 0 < _Count; --_Count) {
+        for (; _Count > 0; --_Count) {
             _Emplace_back_internal(_Val);
         }
 
@@ -964,7 +964,7 @@ public:
             _Newcapacity = _Block_size * _Minimum_map_size;
         }
 
-        if ((empty() && 0 < _Mapsize())
+        if ((empty() && _Mapsize() > 0)
             || (!empty() && size() <= _Newcapacity && _Newcapacity < _Oldcapacity)) { // worth shrinking, do it
             deque _Tmp(_STD make_move_iterator(begin()), _STD make_move_iterator(end()));
             swap(_Tmp);
@@ -976,7 +976,7 @@ public:
             emplace_back();
         }
 
-        while (_Newsize < _Mysize()) {
+        while (_Mysize() > _Newsize) {
             pop_back();
         }
     }
@@ -987,7 +987,7 @@ public:
             _Emplace_back_internal(_Val);
         }
 
-        while (_Newsize < _Mysize()) {
+        while (_Mysize() > _Newsize) {
             pop_back();
         }
     }
@@ -1181,7 +1181,7 @@ public:
         auto _Myfirst       = _Unchecked_begin();
         const auto _Oldsize = _Mysize();
         auto _Assign_count  = (_STD min)(_Count, _Oldsize);
-        for (; 0 < _Assign_count; --_Assign_count) {
+        for (; _Assign_count > 0; --_Assign_count) {
             *_Myfirst = _Val;
             ++_Myfirst;
         }
@@ -1189,7 +1189,7 @@ public:
         const auto _Shrink_by = _Oldsize - _Assign_count;
         auto _Extend_by       = _Count - _Assign_count;
         _Erase_last_n(_Shrink_by);
-        for (; 0 < _Extend_by; --_Extend_by) {
+        for (; _Extend_by > 0; --_Extend_by) {
             _Emplace_back_internal(_Val);
         }
     }
@@ -1304,7 +1304,7 @@ public:
 
         auto _Off   = static_cast<size_type>(_First - begin());
         auto _Count = static_cast<size_type>(_Last - _First);
-        bool _Moved = 0 < _Off && _Off + _Count < _Mysize();
+        bool _Moved = _Off > 0 && _Off + _Count < _Mysize();
 
 #else // _ITERATOR_DEBUG_LEVEL == 2
         auto _Off   = static_cast<size_type>(_First - begin());
@@ -1317,12 +1317,12 @@ public:
 
         if (_Off < static_cast<size_type>(end() - _Last)) { // closer to front
             _STD move_backward(begin(), _First, _Last); // copy over hole
-            for (; 0 < _Count; --_Count) {
+            for (; _Count > 0; --_Count) {
                 pop_front(); // pop copied elements
             }
         } else { // closer to back
             _STD move(_Last, end(), _First); // copy over hole
-            for (; 0 < _Count; --_Count) {
+            for (; _Count > 0; --_Count) {
                 pop_back(); // pop copied elements
             }
         }
@@ -1338,7 +1338,7 @@ public:
 
 private:
     void _Erase_last_n(size_type _Count) noexcept {
-        for (; 0 < _Count; --_Count) {
+        for (; _Count > 0; --_Count) {
             pop_back();
         }
     }
@@ -1376,17 +1376,17 @@ private:
         if (_Off < _Rem) { // closer to front
             _Restore_old_size_guard<_Pop_direction::_Front> _Guard{this, _Oldsize};
             if (_Off < _Count) { // insert longer than prefix
-                for (_Num = _Count - _Off; 0 < _Num; --_Num) {
+                for (_Num = _Count - _Off; _Num > 0; --_Num) {
                     push_front(_Val); // push excess values
                 }
-                for (_Num = _Off; 0 < _Num; --_Num) {
+                for (_Num = _Off; _Num > 0; --_Num) {
                     push_front(begin()[static_cast<difference_type>(_Count - 1)]); // push prefix
                 }
 
                 _Mid = begin() + static_cast<difference_type>(_Count);
                 _STD fill(_Mid, _Mid + static_cast<difference_type>(_Off), _Val); // fill in rest of values
             } else { // insert not longer than prefix
-                for (_Num = _Count; 0 < _Num; --_Num) {
+                for (_Num = _Count; _Num > 0; --_Num) {
                     push_front(begin()[static_cast<difference_type>(_Count - 1)]); // push part of prefix
                 }
 
@@ -1402,7 +1402,7 @@ private:
             _Restore_old_size_guard<_Pop_direction::_Back> _Guard{this, _Oldsize};
             if (_Rem < _Count) { // insert longer than suffix
                 _Orphan_all();
-                for (_Num = _Count - _Rem; 0 < _Num; --_Num) {
+                for (_Num = _Count - _Rem; _Num > 0; --_Num) {
                     _Emplace_back_internal(_Val); // push excess values
                 }
                 for (_Num = 0; _Num < _Rem; ++_Num) {
@@ -1437,10 +1437,10 @@ private:
     }
 
     void _Growmap(size_type _Count) { // grow map by at least _Count pointers, _Mapsize() a power of 2
-        static_assert(1 < _Minimum_map_size, "The _Xlen() test should always be performed.");
+        static_assert(_Minimum_map_size > 1, "The _Xlen() test should always be performed.");
 
         _Alpty _Almap(_Getal());
-        size_type _Newsize = 0 < _Mapsize() ? _Mapsize() : 1;
+        size_type _Newsize = _Mapsize() > 0 ? _Mapsize() : 1;
         while (_Newsize - _Mapsize() < _Count || _Newsize < _Minimum_map_size) {
             // scale _Newsize to 2^N >= _Mapsize() + _Count
             if (max_size() / _Block_size - _Newsize < _Newsize) {
@@ -1466,8 +1466,8 @@ private:
             _Uninitialized_value_construct_n_unchecked1(_Myptr, _Count); // clear rest to initial block
         }
 
-        _Destroy_range(_Map() + _Myboff, _Map() + _Mapsize());
         if (_Map() != _Mapptr()) {
+            _Destroy_range(_Map(), _Map() + _Mapsize());
             _Almap.deallocate(_Map(), _Mapsize()); // free storage for old
         }
 
@@ -1483,14 +1483,14 @@ private:
             pop_back();
         }
 
-        for (size_type _Block = _Mapsize(); 0 < _Block;) { // free storage for a block and destroy pointer
-            if (_Map()[--_Block]) { // free block and destroy its pointer
-                _Getal().deallocate(_Map()[_Block], _Block_size);
-                _Destroy_in_place(_Map()[_Block]);
-            }
-        }
-
         if (_Map() != _Mapptr()) {
+            for (size_type _Block = _Mapsize(); _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
         }
 

tests/std/test.lst

@@ -205,6 +205,7 @@ tests\GH_002581_common_reference_workaround
 tests\GH_002655_alternate_name_broke_linker
 tests\GH_002711_Zc_alignedNew-
 tests\GH_002760_syncstream_memory_leak
+tests\GH_002769_handle_deque_block_pointers
 tests\LWG2597_complex_branch_cut
 tests\LWG3018_shared_ptr_function
 tests\LWG3121_constrained_tuple_forwarding_ctor

tests/std/tests/GH_002769_handle_deque_block_pointers/env.lst

@@ -0,0 +1,4 @@
+# Copyright (c) Microsoft Corporation.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+RUNALL_INCLUDE ..\usual_matrix.lst

tests/std/tests/GH_002769_handle_deque_block_pointers/test.cpp

@@ -0,0 +1,227 @@
+// Copyright (c) Microsoft Corporation.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include <cassert>
+#include <cstddef>
+#include <deque>
+#include <iterator>
+#include <memory>
+#include <type_traits>
+
+using namespace std;
+
+size_t fancy_counter = 0;
+
+template <class T>
+class counting_ptr {
+private:
+    T* p_;
+
+    explicit counting_ptr(T* raw_ptr) noexcept : p_(raw_ptr) {
+        ++fancy_counter;
+    }
+
+    template <class U>
+    friend struct ptr_counting_allocator;
+
+public:
+#ifdef __cpp_lib_concepts
+    using iterator_concept = contiguous_iterator_tag;
+#endif // __cpp_lib_concepts
+    using iterator_category = random_access_iterator_tag;
+    using value_type        = T;
+    using difference_type   = ptrdiff_t;
+    using pointer           = T*;
+    using reference         = add_lvalue_reference_t<T>;
+
+    counting_ptr(nullptr_t) noexcept : counting_ptr{static_cast<T*>(nullptr)} {}
+
+    counting_ptr() noexcept : counting_ptr{nullptr} {}
+
+    counting_ptr(const counting_ptr& other) noexcept : counting_ptr{other.p_} {}
+
+    counting_ptr& operator=(const counting_ptr&) = default;
+
+    counting_ptr& operator=(nullptr_t) noexcept {
+        p_ = nullptr;
+        return *this;
+    }
+
+    ~counting_ptr() {
+        assert(fancy_counter != 0);
+        --fancy_counter;
+    }
+
+    explicit operator bool() const noexcept {
+        return p_ != nullptr;
+    }
+
+    static counting_ptr pointer_to(T& obj) noexcept {
+        return counting_ptr{addressof(obj)};
+    }
+
+    T& operator*() const noexcept {
+        return *p_;
+    }
+
+    T* operator->() const noexcept {
+        return p_;
+    }
+
+    counting_ptr& operator++() noexcept {
+        ++p_;
+        return *this;
+    }
+
+    counting_ptr operator++(int) noexcept {
+        auto tmp = *this;
+        ++p_;
+        return tmp;
+    }
+
+    counting_ptr& operator--() noexcept {
+        --p_;
+        return *this;
+    }
+
+    counting_ptr operator--(int) noexcept {
+        auto tmp = *this;
+        --p_;
+        return tmp;
+    }
+
+    template <class I = ptrdiff_t, enable_if_t<is_integral_v<I>, int> = 0>
+    T& operator[](I n) const noexcept {
+        return p_[n];
+    }
+
+    template <class I = ptrdiff_t, enable_if_t<is_integral_v<I>, int> = 0>
+    counting_ptr& operator+=(I n) noexcept {
+        p_ += n;
+        return *this;
+    }
+
+    template <class I = ptrdiff_t, enable_if_t<is_integral_v<I>, int> = 0>
+    counting_ptr& operator-=(I n) noexcept {
+        p_ -= n;
+        return *this;
+    }
+
+    template <class I = ptrdiff_t, enable_if_t<is_integral_v<I>, int> = 0>
+    friend counting_ptr operator+(const counting_ptr& p, I n) noexcept {
+        auto tmp = p;
+        tmp += n;
+        return tmp;
+    }
+
+    template <class I = ptrdiff_t, enable_if_t<is_integral_v<I>, int> = 0>
+    friend counting_ptr operator+(I n, const counting_ptr& p) noexcept {
+        auto tmp = p;
+        tmp += n;
+        return tmp;
+    }
+
+    template <class I = ptrdiff_t, enable_if_t<is_integral_v<I>, int> = 0>
+    friend counting_ptr operator-(const counting_ptr& p, I n) noexcept {
+        auto tmp = p;
+        tmp -= n;
+        return tmp;
+    }
+
+    friend ptrdiff_t operator-(const counting_ptr& lhs, const counting_ptr& rhs) noexcept {
+        return lhs.p_ - rhs.p_;
+    }
+
+    friend bool operator==(const counting_ptr& p, nullptr_t) noexcept {
+        return p.p_ == nullptr;
+    }
+
+#if _HAS_CXX20
+    friend bool operator==(const counting_ptr& lhs, const counting_ptr& rhs) = default;
+
+    friend auto operator<=>(const counting_ptr& lhs, const counting_ptr& rhs) = default;
+#else // ^^^ _HAS_CXX20 ^^^ / vvv !_HAS_CXX20 vvv
+    friend bool operator==(nullptr_t, const counting_ptr& p) noexcept {
+        return p.p_ == nullptr;
+    }
+
+    friend bool operator!=(const counting_ptr& p, nullptr_t) noexcept {
+        return !(p == nullptr);
+    }
+
+    friend bool operator!=(nullptr_t, const counting_ptr& p) noexcept {
+        return !(p == nullptr);
+    }
+
+    friend bool operator==(const counting_ptr& lhs, const counting_ptr& rhs) noexcept {
+        return lhs.p_ == rhs.p_;
+    }
+
+    friend bool operator!=(const counting_ptr& lhs, const counting_ptr& rhs) noexcept {
+        return !(lhs == rhs);
+    }
+
+    friend bool operator<(const counting_ptr& lhs, const counting_ptr& rhs) noexcept {
+        return lhs.p_ < rhs.p_;
+    }
+
+    friend bool operator>(const counting_ptr& lhs, const counting_ptr& rhs) noexcept {
+        return rhs < lhs;
+    }
+
+    friend bool operator<=(const counting_ptr& lhs, const counting_ptr& rhs) noexcept {
+        return !(rhs < lhs);
+    }
+
+    friend bool operator>=(const counting_ptr& lhs, const counting_ptr& rhs) noexcept {
+        return !(lhs < rhs);
+    }
+#endif // !_HAS_CXX20
+};
+
+template <class T>
+struct ptr_counting_allocator {
+    using value_type      = T;
+    using pointer         = counting_ptr<T>;
+    using size_type       = size_t;
+    using difference_type = ptrdiff_t;
+
+    ptr_counting_allocator() = default;
+
+    template <class U>
+    constexpr ptr_counting_allocator(ptr_counting_allocator<U>) noexcept {}
+
+    pointer allocate(size_type n) {
+        return pointer{allocator<T>{}.allocate(n)};
+    }
+
+    void deallocate(pointer p, size_type n) {
+        allocator<T>{}.deallocate(p.operator->(), n);
+    }
+
+    template <class U>
+    friend constexpr bool operator==(ptr_counting_allocator, ptr_counting_allocator<U>) noexcept {
+        return true;
+    }
+#if !_HAS_CXX20
+    template <class U>
+    friend constexpr bool operator!=(ptr_counting_allocator, ptr_counting_allocator<U>) noexcept {
+        return false;
+    }
+#endif // !_HAS_CXX20
+};
+
+int main() {
+    {
+        deque<int, ptr_counting_allocator<int>> dq{3, 1, 4, 1, 5, 9};
+        dq.insert(dq.end(), {2, 6, 5, 3, 5, 8});
+    }
+    assert(fancy_counter == 0);
+
+    {
+        deque<int, ptr_counting_allocator<int>> dq(979, 323);
+        dq.insert(dq.begin(), 84, 62);
+        dq.erase(dq.begin() + 64, dq.begin() + 338);
+    }
+    assert(fancy_counter == 0);
+}