Skip to content

_Node_handle: destroy with value_type allocator #1310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
CaseyCarter merged 3 commits into from
Oct 9, 2020
Merged

_Node_handle: destroy with value_type allocator #1310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d754b64
_Node_handle: destroy with value_type allocator
CaseyCarter Sep 20, 2020
118762a
Reorder
CaseyCarter Sep 21, 2020
adcaecc
Code review feedback.
StephanTLavavej Sep 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 21 additions & 20 deletions stl/inc/xnode_handle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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));
Expand All @@ -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);
Expand All @@ -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();
Expand All @@ -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<const _Alloc&>(_Alloc_storage);
}

_NODISCARD allocator_type get_allocator() const noexcept /* strengthened */ {
// pre: !empty()
_STL_INTERNAL_CHECK(!empty());
return _Getal();
}

Expand All @@ -169,22 +170,22 @@ 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();
_Construct_in_place(_That._Getal(), _STD move(_Al));
_Destroy_in_place(_Al);
}
} else {
if (!_That._Ptr) {
if (_That._Ptr == nullptr) {
return;
}

Expand All @@ -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};
}
};
Expand Down
165 changes: 161 additions & 4 deletions tests/std/tests/P0083R3_splicing_maps_and_sets/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ struct allocation_guard {
}
};

bool construct_destroy_exact = false;

template <class T>
struct tracked_allocator {
using value_type = T;
Expand All @@ -80,6 +82,26 @@ struct tracked_allocator {
--allocation_count;
}

template <class U, class... Args>
void construct(U* ptr, Args&&... args) {
if constexpr (!std::is_same_v<U, value_type>) {
assert(!construct_destroy_exact);
printf("construct\n");
}
std::allocator<T> alloc;
std::allocator_traits<std::allocator<T>>::construct(alloc, ptr, std::forward<Args>(args)...);
}

template <class U>
void destroy(U* ptr) {
if constexpr (!std::is_same_v<U, value_type>) {
assert(!construct_destroy_exact);
printf("destroy\n");
}
std::allocator<T> alloc;
std::allocator_traits<std::allocator<T>>::destroy(alloc, ptr);
}

template <class U>
bool operator==(tracked_allocator<U> const&) const noexcept {
return true;
Expand Down Expand Up @@ -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<NodeHandle>);
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<NodeHandle>);
Expand Down Expand Up @@ -478,7 +502,7 @@ void test_set() {
{
using S = Set<int, std::less<>, tracked_allocator<int>>;
allocation_guard guard{true};
S s{{0, 1}};
S s{0, 1};
allocation_allowed = false;

auto nh1 = test_extract(s, 42, 0);
Expand Down Expand Up @@ -587,7 +611,7 @@ void test_unordered_set() {
{
using S = Set<int, std::hash<int>, std::equal_to<>, tracked_allocator<int>>;
allocation_guard guard{true};
S s{{0, 1}};
S s{0, 1};
allocation_allowed = false;

auto nh1 = test_extract(s, 42, 0);
Expand Down Expand Up @@ -634,6 +658,137 @@ void test_unordered_set() {
test_merge<Set, std::unordered_multiset>();
}

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<std::pair<int const, char>>;
using M = std::map<int, char, std::less<>, 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<std::pair<int const, char>>;
using M = std::multimap<int, char, std::less<>, 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<int, std::less<>, tracked_allocator<int>>;
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<int, std::less<>, tracked_allocator<int>>;
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<std::pair<int const, char>>;
using M = std::unordered_map<int, char, std::hash<int>, 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<std::pair<int const, char>>;
using M = std::unordered_multimap<int, char, std::hash<int>, 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<int, std::hash<int>, std::equal_to<>, tracked_allocator<int>>;
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<int, std::hash<int>, std::equal_to<>, tracked_allocator<int>>;
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<std::map>();
test_map<std::multimap>();
Expand All @@ -646,4 +801,6 @@ int main() {

test_unordered_set<std::unordered_set>();
test_unordered_set<std::unordered_multiset>();

test_gh1309();
}