diff --git a/stl/inc/xnode_handle.h b/stl/inc/xnode_handle.h index 2c21bc39928..d962579a054 100644 --- a/stl/inc/xnode_handle.h +++ b/stl/inc/xnode_handle.h @@ -80,21 +80,21 @@ class _Node_handle : public _Base<_Node_handle<_Node, _Alloc, _Base, _Types...>, aligned_union_t<0, _Alloc> _Alloc_storage; // Invariant: contains a live _Alloc iff _Ptr != nullptr void _Clear() noexcept { // destroy any contained node and return to the empty state - if (_Ptr) { + if (_Ptr != nullptr) { _Alloc& _Al = _Getal(); + _Alty_traits::destroy(_Al, _STD addressof(_Ptr->_Myval)); _Alnode _Node_alloc{_Al}; - _Alnode_traits::destroy(_Node_alloc, _STD addressof(_Ptr->_Myval)); _Node::_Freenode0(_Node_alloc, _Ptr); _Destroy_in_place(_Al); _Ptr = nullptr; } } - _Node_handle(const _Nodeptr _My_ptr, const _Alloc& _My_alloc) noexcept - : _Ptr{_My_ptr} { // Initialize a _Node_handle that holds the specified node - // pre: _My_ptr != nullptr - // pre: _Alloc can release _Ptr - _Construct_in_place(_Getal(), _My_alloc); + _Node_handle(const _Nodeptr _Ptr_, const _Alloc& _Al) noexcept + : _Ptr{_Ptr_} { // Initialize a _Node_handle that holds the specified node + // pre: _Alloc can release _Ptr + _STL_INTERNAL_CHECK(_Ptr_ != nullptr); + _Construct_in_place(_Getal(), _Al); } public: @@ -105,7 +105,7 @@ class _Node_handle : public _Base<_Node_handle<_Node, _Alloc, _Base, _Types...>, } _Node_handle(_Node_handle&& _That) noexcept : _Ptr{_That._Ptr} { // steal node and allocator (if any) from _That - if (_Ptr) { + if (_Ptr != nullptr) { _That._Ptr = nullptr; _Alloc& _That_al = _That._Getal(); _Construct_in_place(_Getal(), _STD move(_That_al)); @@ -115,8 +115,8 @@ class _Node_handle : public _Base<_Node_handle<_Node, _Alloc, _Base, _Types...>, _Node_handle& operator=(_Node_handle&& _That) noexcept /* strengthened */ { // steal state from _That - if (!_Ptr) { - if (_That._Ptr) { + if (_Ptr == nullptr) { + if (_That._Ptr != nullptr) { _Alloc& _That_al = _That._Getal(); _Construct_in_place(_Getal(), _STD move(_That_al)); _Destroy_in_place(_That_al); @@ -126,14 +126,14 @@ class _Node_handle : public _Base<_Node_handle<_Node, _Alloc, _Base, _Types...>, return *this; } - if (!_That._Ptr || this == _STD addressof(_That)) { + if (_That._Ptr == nullptr || this == _STD addressof(_That)) { _Clear(); return *this; } _Alloc& _Al = _Getal(); + _Alty_traits::destroy(_Al, _STD addressof(_Ptr->_Myval)); _Alnode _Node_alloc{_Al}; - _Alnode_traits::destroy(_Node_alloc, _STD addressof(_Ptr->_Myval)); _Alnode_traits::deallocate(_Node_alloc, _Ptr, 1); _Alloc& _That_al = _That._Getal(); @@ -148,15 +148,16 @@ class _Node_handle : public _Base<_Node_handle<_Node, _Alloc, _Base, _Types...>, return _Ptr; } - _Alloc& _Getal() noexcept { // pre: !empty() + _Alloc& _Getal() noexcept { return reinterpret_cast<_Alloc&>(_Alloc_storage); } - const _Alloc& _Getal() const noexcept { // pre: !empty() + const _Alloc& _Getal() const noexcept { + _STL_INTERNAL_CHECK(!empty()); return reinterpret_cast(_Alloc_storage); } _NODISCARD allocator_type get_allocator() const noexcept /* strengthened */ { - // pre: !empty() + _STL_INTERNAL_CHECK(!empty()); return _Getal(); } @@ -169,14 +170,14 @@ class _Node_handle : public _Base<_Node_handle<_Node, _Alloc, _Base, _Types...>, } _Nodeptr _Release() noexcept { // extract the node from *this - // pre: !empty() + _STL_INTERNAL_CHECK(!empty()); _Destroy_in_place(_Getal()); return _STD exchange(_Ptr, nullptr); } void swap(_Node_handle& _That) noexcept /* strengthened */ { - if (_Ptr) { - if (_That._Ptr) { + if (_Ptr != nullptr) { + if (_That._Ptr != nullptr) { _Pocs(_Getal(), _That._Getal()); } else { _Alloc& _Al = _Getal(); @@ -184,7 +185,7 @@ class _Node_handle : public _Base<_Node_handle<_Node, _Alloc, _Base, _Types...>, _Destroy_in_place(_Al); } } else { - if (!_That._Ptr) { + if (_That._Ptr == nullptr) { return; } @@ -200,8 +201,8 @@ class _Node_handle : public _Base<_Node_handle<_Node, _Alloc, _Base, _Types...>, static _Node_handle _Make(const _Nodeptr _Ptr, const allocator_type& _Al) { // initialize a _Node_handle that holds _Ptr and _Al - // pre: _Ptr != nullptr // pre: _Al can release _Ptr + _STL_INTERNAL_CHECK(_Ptr != nullptr); return _Node_handle{_Ptr, _Al}; } }; diff --git a/tests/std/tests/P0083R3_splicing_maps_and_sets/test.cpp b/tests/std/tests/P0083R3_splicing_maps_and_sets/test.cpp index f6eef6d8d92..95cbc6c9f58 100644 --- a/tests/std/tests/P0083R3_splicing_maps_and_sets/test.cpp +++ b/tests/std/tests/P0083R3_splicing_maps_and_sets/test.cpp @@ -59,6 +59,8 @@ struct allocation_guard { } }; +bool construct_destroy_exact = false; + template struct tracked_allocator { using value_type = T; @@ -80,6 +82,26 @@ struct tracked_allocator { --allocation_count; } + template + void construct(U* ptr, Args&&... args) { + if constexpr (!std::is_same_v) { + assert(!construct_destroy_exact); + printf("construct\n"); + } + std::allocator alloc; + std::allocator_traits>::construct(alloc, ptr, std::forward(args)...); + } + + template + void destroy(U* ptr) { + if constexpr (!std::is_same_v) { + assert(!construct_destroy_exact); + printf("destroy\n"); + } + std::allocator alloc; + std::allocator_traits>::destroy(alloc, ptr); + } + template bool operator==(tracked_allocator const&) const noexcept { return true; @@ -131,9 +153,11 @@ void test_node_handle(NodeHandle& nh1, NodeHandle& nh2, Validator1 v1, Validator // Nothrow/constexpr default construction static_assert(std::is_nothrow_default_constructible_v); CHECK_EMPTY(NodeHandle{}); -#ifdef __clang__ +#if defined(__cpp_constinit) + { static constinit NodeHandle static_handle{}; } +#elif defined(__clang__) { [[clang::require_constant_initialization]] static NodeHandle static_handle{}; } -#endif // __clang__ +#endif // ^^^ __clang__ ^^^ // No copies! static_assert(!std::is_copy_constructible_v); @@ -478,7 +502,7 @@ void test_set() { { using S = Set, tracked_allocator>; allocation_guard guard{true}; - S s{{0, 1}}; + S s{0, 1}; allocation_allowed = false; auto nh1 = test_extract(s, 42, 0); @@ -587,7 +611,7 @@ void test_unordered_set() { { using S = Set, std::equal_to<>, tracked_allocator>; allocation_guard guard{true}; - S s{{0, 1}}; + S s{0, 1}; allocation_allowed = false; auto nh1 = test_extract(s, 42, 0); @@ -634,6 +658,137 @@ void test_unordered_set() { test_merge(); } +void test_gh1309() { + // Guard against regression of GH-1309, in which node handles were incorrectly destroying the user value with a node + // allocator rather than a value_type allocator as the Standard requires. + + allocation_guard guard{true}; + + { + using A = tracked_allocator>; + using M = std::map, A>; + using NH = M::node_type; + NH nh1; + NH nh2; + { + M m{{0, 'x'}, {1, 'y'}}; + nh1 = m.extract(0); + nh2 = m.extract(1); + } + construct_destroy_exact = true; + nh1 = std::move(nh2); + } + construct_destroy_exact = false; + + { + using A = tracked_allocator>; + using M = std::multimap, A>; + using NH = M::node_type; + NH nh1; + NH nh2; + { + M m{{0, 'x'}, {0, 'y'}}; + nh1 = m.extract(0); + nh2 = m.extract(0); + } + construct_destroy_exact = true; + nh1 = std::move(nh2); + } + construct_destroy_exact = false; + + { + using S = std::set, tracked_allocator>; + using NH = S::node_type; + NH nh1; + NH nh2; + { + S s{0, 1}; + nh1 = s.extract(0); + nh2 = s.extract(s.begin()); + } + construct_destroy_exact = true; + nh1 = std::move(nh2); + } + construct_destroy_exact = false; + + { + using S = std::multiset, tracked_allocator>; + using NH = S::node_type; + NH nh1; + NH nh2; + { + S s{0, 0}; + nh1 = s.extract(0); + nh2 = s.extract(s.begin()); + } + construct_destroy_exact = true; + nh1 = std::move(nh2); + } + construct_destroy_exact = false; + + { + using A = tracked_allocator>; + using M = std::unordered_map, std::equal_to<>, A>; + using NH = M::node_type; + NH nh1; + NH nh2; + { + M m{{0, 'x'}, {1, 'y'}}; + nh1 = m.extract(0); + nh2 = m.extract(m.begin()); + } + construct_destroy_exact = true; + nh1 = std::move(nh2); + } + construct_destroy_exact = false; + + { + using A = tracked_allocator>; + using M = std::unordered_multimap, std::equal_to<>, A>; + using NH = M::node_type; + NH nh1; + NH nh2; + { + M m{{0, 'x'}, {0, 'y'}}; + nh1 = m.extract(0); + nh2 = m.extract(m.begin()); + } + construct_destroy_exact = true; + nh1 = std::move(nh2); + } + construct_destroy_exact = false; + + { + using S = std::unordered_set, std::equal_to<>, tracked_allocator>; + using NH = S::node_type; + NH nh1; + NH nh2; + { + S s{0, 1}; + nh1 = s.extract(0); + nh2 = s.extract(s.begin()); + } + construct_destroy_exact = true; + nh1 = std::move(nh2); + } + construct_destroy_exact = false; + + { + using S = std::unordered_multiset, std::equal_to<>, tracked_allocator>; + using NH = S::node_type; + NH nh1; + NH nh2; + { + S s{0, 0}; + nh1 = s.extract(0); + nh2 = s.extract(s.begin()); + } + construct_destroy_exact = true; + nh1 = std::move(nh2); + } + construct_destroy_exact = false; +} + int main() { test_map(); test_map(); @@ -646,4 +801,6 @@ int main() { test_unordered_set(); test_unordered_set(); + + test_gh1309(); }