Skip to content
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

Untag dispatch tree #2287

Merged
merged 5 commits into from
Mar 19, 2022
Merged

Untag dispatch tree #2287

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
28b0d06
untag dispatch tree
AlexGuteniev Oct 20, 2021
641dc45
++
AlexGuteniev Oct 20, 2021
4b42277
@miscco's strategic suggestion
AlexGuteniev Oct 20, 2021
7e4df46
Merge remote-tracking branch 'upstream/main' into untag_dispatch_tree
AlexGuteniev Dec 18, 2021
b6bcc1b
Merge remote-tracking branch 'upstream/main' into untag_dispatch_tree
AlexGuteniev Jan 12, 2022
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
74 changes: 34 additions & 40 deletions stl/inc/xtree
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,7 @@ struct _Tree_temp_node : _Tree_temp_node_alloc<_Alnode> {
template <class _Traits>
class _Tree { // ordered red-black tree for map/multimap/set/multiset
public:
using key_type = typename _Traits::key_type;
using value_type = typename _Traits::value_type;
using allocator_type = typename _Traits::allocator_type;

Expand All @@ -842,15 +843,15 @@ protected:
typename _Alty_traits::pointer, typename _Alty_traits::const_pointer, value_type&, const value_type&,
_Nodeptr>>>;

static constexpr bool _Multi = _Traits::_Multi;
static constexpr bool _Multi = _Traits::_Multi;
static constexpr bool _Is_set = is_same_v<key_type, value_type>;

enum _Redbl { // colors for link to parent
_Red,
_Black
};

public:
using key_type = typename _Traits::key_type;
using value_compare = typename _Traits::value_compare;

using key_compare = typename _Traits::key_compare;
Expand All @@ -862,22 +863,18 @@ public:
using reference = value_type&;
using const_reference = const value_type&;

using iterator =
conditional_t<is_same_v<key_type, value_type>, _Tree_const_iterator<_Scary_val>, _Tree_iterator<_Scary_val>>;
using const_iterator = _Tree_const_iterator<_Scary_val>;
using _Unchecked_iterator = conditional_t<is_same_v<key_type, value_type>,
_Tree_unchecked_const_iterator<_Scary_val>, _Tree_unchecked_iterator<_Scary_val>>;
using iterator = conditional_t<_Is_set, _Tree_const_iterator<_Scary_val>, _Tree_iterator<_Scary_val>>;
using const_iterator = _Tree_const_iterator<_Scary_val>;
using _Unchecked_iterator =
conditional_t<_Is_set, _Tree_unchecked_const_iterator<_Scary_val>, _Tree_unchecked_iterator<_Scary_val>>;
using _Unchecked_const_iterator = _Tree_unchecked_const_iterator<_Scary_val>;

using reverse_iterator = _STD reverse_iterator<iterator>;
using const_reverse_iterator = _STD reverse_iterator<const_iterator>;

struct _Copy_tag {
explicit _Copy_tag() = default;
};

struct _Move_tag {
explicit _Move_tag() = default;
enum class _Strategy : bool {
_Copy,
_Move,
};

_Tree(const key_compare& _Parg) : _Mypair(_One_then_variadic_args_t{}, _Parg, _Zero_then_variadic_args_t{}) {
Expand All @@ -897,7 +894,7 @@ public:
const auto _Scary = _Get_scary();
_Container_proxy_ptr<_Alty> _Proxy(_Alproxy, *_Scary);
_Tree_head_scoped_ptr<_Alnode, _Scary_val> _Sentinel(_Getal(), *_Scary);
_Copy(_Right, _Copy_tag{});
_Copy<_Strategy::_Copy>(_Right);
_Sentinel._Release();
_Proxy._Release();
}
Expand All @@ -916,7 +913,7 @@ private:
const auto _Scary = _Get_scary();
_Container_proxy_ptr<_Alty> _Proxy(_Alproxy, *_Scary);
_Tree_head_scoped_ptr<_Alnode, _Scary_val> _Sentinel(_Getal(), *_Scary);
_Copy(_Right, _Move_tag{});
_Copy<_Strategy::_Move>(_Right);
_Sentinel._Release();
_Proxy._Release();
}
Expand Down Expand Up @@ -969,7 +966,7 @@ public:
if (_Al != _Right_al) {
clear();
_Getcomp() = _Right._Getcomp(); // intentionally copy comparator, see LWG-2227
_Copy(_Right, _Move_tag{});
_Copy<_Strategy::_Move>(_Right);
return *this;
}
}
Expand Down Expand Up @@ -1110,15 +1107,15 @@ public:
_Scary->_Myhead = _Newhead;
_Proxy._Bind(_Alproxy, _Scary);
_Getcomp() = _Right._Getcomp();
_Copy(_Right, _Copy_tag{});
_Copy<_Strategy::_Copy>(_Right);
return *this;
}
}

clear();
_Getcomp() = _Right._Getcomp();
_Pocca(_Al, _Right_al);
_Copy(_Right, _Copy_tag{});
_Copy<_Strategy::_Copy>(_Right);

return *this;
}
Expand Down Expand Up @@ -1621,11 +1618,11 @@ protected:
}
}

template <class _Moveit>
void _Copy(const _Tree& _Right, _Moveit _Movefl) { // copy or move entire tree from _Right
template <_Strategy _Strat>
void _Copy(const _Tree& _Right) { // copy or move entire tree from _Right
const auto _Scary = _Get_scary();
const auto _Right_scary = _Right._Get_scary();
_Scary->_Myhead->_Parent = _Copy_nodes(_Right_scary->_Myhead->_Parent, _Scary->_Myhead, _Movefl);
_Scary->_Myhead->_Parent = _Copy_nodes<_Strat>(_Right_scary->_Myhead->_Parent, _Scary->_Myhead);
_Scary->_Mysize = _Right_scary->_Mysize;
if (!_Scary->_Myhead->_Parent->_Isnil) { // nonempty tree, look for new smallest and largest
_Scary->_Myhead->_Left = _Scary_val::_Min(_Scary->_Myhead->_Parent);
Expand All @@ -1636,39 +1633,36 @@ protected:
}
}

template <class _Ty, class _Is_set>
_Nodeptr _Copy_or_move(_Ty& _Val, _Copy_tag, _Is_set) { // copy to new node
return _Buynode(_Val);
}

template <class _Ty>
_Nodeptr _Copy_or_move(_Ty& _Val, _Move_tag, true_type) { // move to new node -- set
return _Buynode(_STD move(_Val));
}

template <class _Ty>
_Nodeptr _Copy_or_move(_Ty& _Val, _Move_tag, false_type) { // move to new node -- map
return _Buynode(_STD move(const_cast<key_type&>(_Val.first)), _STD move(_Val.second));
template <_Strategy _Strat, class _Ty>
_Nodeptr _Copy_or_move(_Ty& _Val) {
if constexpr (_Strat == _Strategy::_Copy) {
return _Buynode(_Val);
} else {
if constexpr (_Is_set) {
return _Buynode(_STD move(_Val));
} else {
return _Buynode(_STD move(const_cast<key_type&>(_Val.first)), _STD move(_Val.second));
}
}
Comment on lines +1640 to +1646
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can flatten that into one else if

Suggested change
} else {
if constexpr (_Is_set) {
return _Buynode(_STD move(_Val));
} else {
return _Buynode(_STD move(const_cast<key_type&>(_Val.first)), _STD move(_Val.second));
}
}
} else if constexpr (_Is_set) {
return _Buynode(_STD move(_Val));
} else {
return _Buynode(_STD move(const_cast<key_type&>(_Val.first)), _STD move(_Val.second));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think unflatten is more clear

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah not a hill I would die on, feel free to disregard

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I'll wait for other reviewers opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're pretty close, but I agree with Alex that the unflattened structure is clearer. The rationale is that this is a forked path, with a sub-fork, which is shown by the unflattened structure. It would be equally clear (ignoring our "prefer positive tests" convention) if the branches were reversed. Flattening the logic would make the order of the tests important, and having to worry about the order in which tests are performed is a slight mental burden.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have a mild preference to unflatten, so it's a tie, and the status quo code in the PR stays.

}

template <class _Moveit>
_Nodeptr _Copy_nodes(_Nodeptr _Rootnode, _Nodeptr _Wherenode, _Moveit _Movefl) {
template <_Strategy _Strat>
_Nodeptr _Copy_nodes(_Nodeptr _Rootnode, _Nodeptr _Wherenode) {
// copy entire subtree, recursively
const auto _Scary = _Get_scary();
_Nodeptr _Newroot = _Scary->_Myhead; // point at nil node

if (!_Rootnode->_Isnil) { // copy or move a node, then any subtrees
bool_constant<is_same_v<key_type, value_type>> _Is_set;
_Nodeptr _Pnode = _Copy_or_move(_Rootnode->_Myval, _Movefl, _Is_set);
_Nodeptr _Pnode = _Copy_or_move<_Strat>(_Rootnode->_Myval);
_Pnode->_Parent = _Wherenode;
_Pnode->_Color = _Rootnode->_Color;
if (_Newroot->_Isnil) {
_Newroot = _Pnode; // memorize new root
}

_TRY_BEGIN
_Pnode->_Left = _Copy_nodes(_Rootnode->_Left, _Pnode, _Movefl);
_Pnode->_Right = _Copy_nodes(_Rootnode->_Right, _Pnode, _Movefl);
_Pnode->_Left = _Copy_nodes<_Strat>(_Rootnode->_Left, _Pnode);
_Pnode->_Right = _Copy_nodes<_Strat>(_Rootnode->_Right, _Pnode);
_CATCH_ALL
_Scary->_Erase_tree_and_orphan(_Getal(), _Newroot); // subtree copy failed, bail out
_RERAISE;
Expand Down