-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[libc++] Refactor __tree::__find_equal to not have an out parameter #147345
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.
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
Open
philnik777
wants to merge
1
commit into
llvm:main
Choose a base branch
from
philnik777:refactor_find_equal
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+86
−104
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ With the latest revision this PR passed the C/C++ code formatter. |
0b1dda6
to
eef8948
Compare
97ca009
to
aa226b2
Compare
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesFull diff: https://github.com/llvm/llvm-project/pull/147345.diff 2 Files Affected:
diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index a84a0e43d3dda..778cac6ee7766 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -1045,8 +1045,7 @@ public:
template <class _Key>
_LIBCPP_HIDE_FROM_ABI iterator find(const _Key& __key) {
- __end_node_pointer __parent;
- __node_base_pointer __match = __find_equal(__parent, __key);
+ auto [__, __match] = __find_equal(__key);
if (__match == nullptr)
return end();
return iterator(static_cast<__node_pointer>(__match));
@@ -1054,8 +1053,7 @@ public:
template <class _Key>
_LIBCPP_HIDE_FROM_ABI const_iterator find(const _Key& __key) const {
- __end_node_pointer __parent;
- __node_base_pointer __match = __find_equal(__parent, __key);
+ auto [__, __match] = __find_equal(__key);
if (__match == nullptr)
return end();
return const_iterator(static_cast<__node_pointer>(__match));
@@ -1109,15 +1107,89 @@ public:
// FIXME: Make this function const qualified. Unfortunately doing so
// breaks existing code which uses non-const callable comparators.
+
+ // Find place to insert if __v doesn't exist
+ // Set __parent to parent of null leaf
+ // Return reference to null leaf
+ // If __v exists, set parent to node of __v and return reference to node of __v
template <class _Key>
- _LIBCPP_HIDE_FROM_ABI __node_base_pointer& __find_equal(__end_node_pointer& __parent, const _Key& __v);
+ _LIBCPP_HIDE_FROM_ABI pair<__end_node_pointer, __node_base_pointer&> __find_equal(const _Key& __v) {
+ using _Pair = pair<__end_node_pointer, __node_base_pointer&>;
+
+ __node_pointer __nd = __root();
+
+ if (__nd == nullptr) {
+ auto __end = __end_node();
+ return _Pair(__end, __end->__left_);
+ }
+
+ __node_base_pointer* __node_ptr = __root_ptr();
+ while (true) {
+ if (value_comp()(__v, __nd->__get_value())) {
+ if (__nd->__left_ == nullptr)
+ return _Pair(static_cast<__end_node_pointer>(__nd), __nd->__left_);
+
+ __node_ptr = std::addressof(__nd->__left_);
+ __nd = static_cast<__node_pointer>(__nd->__left_);
+ } else if (value_comp()(__nd->__get_value(), __v)) {
+ if (__nd->__right_ == nullptr)
+ return _Pair(static_cast<__end_node_pointer>(__nd), __nd->__right_);
+
+ __node_ptr = std::addressof(__nd->__right_);
+ __nd = static_cast<__node_pointer>(__nd->__right_);
+ } else {
+ return _Pair(static_cast<__end_node_pointer>(__nd), *__node_ptr);
+ }
+ }
+ }
+
template <class _Key>
- _LIBCPP_HIDE_FROM_ABI __node_base_pointer& __find_equal(__end_node_pointer& __parent, const _Key& __v) const {
- return const_cast<__tree*>(this)->__find_equal(__parent, __v);
+ _LIBCPP_HIDE_FROM_ABI pair<__end_node_pointer, __node_base_pointer&> __find_equal(const _Key& __v) const {
+ return const_cast<__tree*>(this)->__find_equal(__v);
}
+
+ // Find place to insert if __v doesn't exist
+ // First check prior to __hint.
+ // Next check after __hint.
+ // Next do O(log N) search.
+ // Set __parent to parent of null leaf
+ // Return reference to null leaf
+ // If __v exists, set parent to node of __v and return reference to node of __v
template <class _Key>
- _LIBCPP_HIDE_FROM_ABI __node_base_pointer&
- __find_equal(const_iterator __hint, __end_node_pointer& __parent, __node_base_pointer& __dummy, const _Key& __v);
+ _LIBCPP_HIDE_FROM_ABI pair<__end_node_pointer, __node_base_pointer&>
+ __find_equal(const_iterator __hint, __node_base_pointer& __dummy, const _Key& __v) {
+ using _Pair = pair<__end_node_pointer, __node_base_pointer&>;
+
+ if (__hint == end() || value_comp()(__v, *__hint)) { // check before
+ // __v < *__hint
+ const_iterator __prior = __hint;
+ if (__prior == begin() || value_comp()(*--__prior, __v)) {
+ // *prev(__hint) < __v < *__hint
+ if (__hint.__ptr_->__left_ == nullptr)
+ return _Pair(__hint.__ptr_, __hint.__ptr_->__left_);
+ return _Pair(__prior.__ptr_, static_cast<__node_pointer>(__prior.__ptr_)->__right_);
+ }
+ // __v <= *prev(__hint)
+ return __find_equal(__v);
+ }
+
+ if (value_comp()(*__hint, __v)) { // check after
+ // *__hint < __v
+ const_iterator __next = std::next(__hint);
+ if (__next == end() || value_comp()(__v, *__next)) {
+ // *__hint < __v < *std::next(__hint)
+ if (__hint.__get_np()->__right_ == nullptr)
+ return _Pair(__hint.__ptr_, static_cast<__node_pointer>(__hint.__ptr_)->__right_);
+ return _Pair(__next.__ptr_, __next.__ptr_->__left_);
+ }
+ // *next(__hint) <= __v
+ return __find_equal(__v);
+ }
+
+ // else __v == *__hint
+ __dummy = static_cast<__node_base_pointer>(__hint.__ptr_);
+ return _Pair(__hint.__ptr_, __dummy);
+ }
_LIBCPP_HIDE_FROM_ABI void __copy_assign_alloc(const __tree& __t) {
__copy_assign_alloc(__t, integral_constant<bool, __node_traits::propagate_on_container_copy_assignment::value>());
@@ -1670,94 +1742,6 @@ typename __tree<_Tp, _Compare, _Allocator>::__node_base_pointer& __tree<_Tp, _Co
return __find_leaf_low(__parent, __v);
}
-// Find place to insert if __v doesn't exist
-// Set __parent to parent of null leaf
-// Return reference to null leaf
-// If __v exists, set parent to node of __v and return reference to node of __v
-template <class _Tp, class _Compare, class _Allocator>
-template <class _Key>
-typename __tree<_Tp, _Compare, _Allocator>::__node_base_pointer&
-__tree<_Tp, _Compare, _Allocator>::__find_equal(__end_node_pointer& __parent, const _Key& __v) {
- __node_pointer __nd = __root();
- __node_base_pointer* __nd_ptr = __root_ptr();
- if (__nd != nullptr) {
- while (true) {
- if (value_comp()(__v, __nd->__get_value())) {
- if (__nd->__left_ != nullptr) {
- __nd_ptr = std::addressof(__nd->__left_);
- __nd = static_cast<__node_pointer>(__nd->__left_);
- } else {
- __parent = static_cast<__end_node_pointer>(__nd);
- return __parent->__left_;
- }
- } else if (value_comp()(__nd->__get_value(), __v)) {
- if (__nd->__right_ != nullptr) {
- __nd_ptr = std::addressof(__nd->__right_);
- __nd = static_cast<__node_pointer>(__nd->__right_);
- } else {
- __parent = static_cast<__end_node_pointer>(__nd);
- return __nd->__right_;
- }
- } else {
- __parent = static_cast<__end_node_pointer>(__nd);
- return *__nd_ptr;
- }
- }
- }
- __parent = __end_node();
- return __parent->__left_;
-}
-
-// Find place to insert if __v doesn't exist
-// First check prior to __hint.
-// Next check after __hint.
-// Next do O(log N) search.
-// Set __parent to parent of null leaf
-// Return reference to null leaf
-// If __v exists, set parent to node of __v and return reference to node of __v
-template <class _Tp, class _Compare, class _Allocator>
-template <class _Key>
-typename __tree<_Tp, _Compare, _Allocator>::__node_base_pointer& __tree<_Tp, _Compare, _Allocator>::__find_equal(
- const_iterator __hint, __end_node_pointer& __parent, __node_base_pointer& __dummy, const _Key& __v) {
- if (__hint == end() || value_comp()(__v, *__hint)) // check before
- {
- // __v < *__hint
- const_iterator __prior = __hint;
- if (__prior == begin() || value_comp()(*--__prior, __v)) {
- // *prev(__hint) < __v < *__hint
- if (__hint.__ptr_->__left_ == nullptr) {
- __parent = __hint.__ptr_;
- return __parent->__left_;
- } else {
- __parent = __prior.__ptr_;
- return static_cast<__node_base_pointer>(__prior.__ptr_)->__right_;
- }
- }
- // __v <= *prev(__hint)
- return __find_equal(__parent, __v);
- } else if (value_comp()(*__hint, __v)) // check after
- {
- // *__hint < __v
- const_iterator __next = std::next(__hint);
- if (__next == end() || value_comp()(__v, *__next)) {
- // *__hint < __v < *std::next(__hint)
- if (__hint.__get_np()->__right_ == nullptr) {
- __parent = __hint.__ptr_;
- return static_cast<__node_base_pointer>(__hint.__ptr_)->__right_;
- } else {
- __parent = __next.__ptr_;
- return __parent->__left_;
- }
- }
- // *next(__hint) <= __v
- return __find_equal(__parent, __v);
- }
- // else __v == *__hint
- __parent = __hint.__ptr_;
- __dummy = static_cast<__node_base_pointer>(__hint.__ptr_);
- return __dummy;
-}
-
template <class _Tp, class _Compare, class _Allocator>
void __tree<_Tp, _Compare, _Allocator>::__insert_node_at(
__end_node_pointer __parent, __node_base_pointer& __child, __node_base_pointer __new_node) _NOEXCEPT {
@@ -1776,10 +1760,9 @@ template <class _Tp, class _Compare, class _Allocator>
template <class _Key, class... _Args>
pair<typename __tree<_Tp, _Compare, _Allocator>::iterator, bool>
__tree<_Tp, _Compare, _Allocator>::__emplace_unique_key_args(_Key const& __k, _Args&&... __args) {
- __end_node_pointer __parent;
- __node_base_pointer& __child = __find_equal(__parent, __k);
- __node_pointer __r = static_cast<__node_pointer>(__child);
- bool __inserted = false;
+ auto [__parent, __child] = __find_equal(__k);
+ __node_pointer __r = static_cast<__node_pointer>(__child);
+ bool __inserted = false;
if (__child == nullptr) {
__node_holder __h = __construct_node(std::forward<_Args>(__args)...);
__insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__h.get()));
@@ -1794,11 +1777,10 @@ template <class _Key, class... _Args>
pair<typename __tree<_Tp, _Compare, _Allocator>::iterator, bool>
__tree<_Tp, _Compare, _Allocator>::__emplace_hint_unique_key_args(
const_iterator __p, _Key const& __k, _Args&&... __args) {
- __end_node_pointer __parent;
__node_base_pointer __dummy;
- __node_base_pointer& __child = __find_equal(__p, __parent, __dummy, __k);
- __node_pointer __r = static_cast<__node_pointer>(__child);
- bool __inserted = false;
+ auto [__parent, __child] = __find_equal(__p, __dummy, __k);
+ __node_pointer __r = static_cast<__node_pointer>(__child);
+ bool __inserted = false;
if (__child == nullptr) {
__node_holder __h = __construct_node(std::forward<_Args>(__args)...);
__insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__h.get()));
@@ -1823,11 +1805,10 @@ template <class _Tp, class _Compare, class _Allocator>
template <class... _Args>
pair<typename __tree<_Tp, _Compare, _Allocator>::iterator, bool>
__tree<_Tp, _Compare, _Allocator>::__emplace_unique_impl(_Args&&... __args) {
- __node_holder __h = __construct_node(std::forward<_Args>(__args)...);
- __end_node_pointer __parent;
- __node_base_pointer& __child = __find_equal(__parent, __h->__get_value());
- __node_pointer __r = static_cast<__node_pointer>(__child);
- bool __inserted = false;
+ __node_holder __h = __construct_node(std::forward<_Args>(__args)...);
+ auto [__parent, __child] = __find_equal(__h->__get_value());
+ __node_pointer __r = static_cast<__node_pointer>(__child);
+ bool __inserted = false;
if (__child == nullptr) {
__insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__h.get()));
__r = __h.release();
@@ -1841,10 +1822,9 @@ template <class... _Args>
typename __tree<_Tp, _Compare, _Allocator>::iterator
__tree<_Tp, _Compare, _Allocator>::__emplace_hint_unique_impl(const_iterator __p, _Args&&... __args) {
__node_holder __h = __construct_node(std::forward<_Args>(__args)...);
- __end_node_pointer __parent;
__node_base_pointer __dummy;
- __node_base_pointer& __child = __find_equal(__p, __parent, __dummy, __h->__get_value());
- __node_pointer __r = static_cast<__node_pointer>(__child);
+ auto [__parent, __child] = __find_equal(__p, __dummy, __h->__get_value());
+ __node_pointer __r = static_cast<__node_pointer>(__child);
if (__child == nullptr) {
__insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__h.get()));
__r = __h.release();
@@ -1877,10 +1857,9 @@ __tree<_Tp, _Compare, _Allocator>::__emplace_hint_multi(const_iterator __p, _Arg
template <class _Tp, class _Compare, class _Allocator>
pair<typename __tree<_Tp, _Compare, _Allocator>::iterator, bool>
__tree<_Tp, _Compare, _Allocator>::__node_assign_unique(const value_type& __v, __node_pointer __nd) {
- __end_node_pointer __parent;
- __node_base_pointer& __child = __find_equal(__parent, __v);
- __node_pointer __r = static_cast<__node_pointer>(__child);
- bool __inserted = false;
+ auto [__parent, __child] = __find_equal(__v);
+ __node_pointer __r = static_cast<__node_pointer>(__child);
+ bool __inserted = false;
if (__child == nullptr) {
__assign_value(__nd->__get_value(), __v);
__insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__nd));
@@ -1929,8 +1908,7 @@ __tree<_Tp, _Compare, _Allocator>::__node_handle_insert_unique(_NodeHandle&& __n
return _InsertReturnType{end(), false, _NodeHandle()};
__node_pointer __ptr = __nh.__ptr_;
- __end_node_pointer __parent;
- __node_base_pointer& __child = __find_equal(__parent, __ptr->__get_value());
+ auto [__parent, __child] = __find_equal(__ptr->__get_value());
if (__child != nullptr)
return _InsertReturnType{iterator(static_cast<__node_pointer>(__child)), false, std::move(__nh)};
@@ -1947,10 +1925,9 @@ __tree<_Tp, _Compare, _Allocator>::__node_handle_insert_unique(const_iterator __
return end();
__node_pointer __ptr = __nh.__ptr_;
- __end_node_pointer __parent;
__node_base_pointer __dummy;
- __node_base_pointer& __child = __find_equal(__hint, __parent, __dummy, __ptr->__get_value());
- __node_pointer __r = static_cast<__node_pointer>(__child);
+ auto [__parent, __child] = __find_equal(__hint, __dummy, __ptr->__get_value());
+ __node_pointer __r = static_cast<__node_pointer>(__child);
if (__child == nullptr) {
__insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__ptr));
__r = __ptr;
@@ -1983,8 +1960,7 @@ _LIBCPP_HIDE_FROM_ABI void __tree<_Tp, _Compare, _Allocator>::__node_handle_merg
for (typename _Tree::iterator __i = __source.begin(); __i != __source.end();) {
__node_pointer __src_ptr = __i.__get_np();
- __end_node_pointer __parent;
- __node_base_pointer& __child = __find_equal(__parent, __src_ptr->__get_value());
+ auto [__parent, __child] = __find_equal(__src_ptr->__get_value());
++__i;
if (__child != nullptr)
continue;
diff --git a/libcxx/include/map b/libcxx/include/map
index 4dfce70e50e7f..9b54903b5b334 100644
--- a/libcxx/include/map
+++ b/libcxx/include/map
@@ -1429,9 +1429,8 @@ map<_Key, _Tp, _Compare, _Allocator>::__construct_node_with_key(const key_type&
template <class _Key, class _Tp, class _Compare, class _Allocator>
_Tp& map<_Key, _Tp, _Compare, _Allocator>::operator[](const key_type& __k) {
- __parent_pointer __parent;
- __node_base_pointer& __child = __tree_.__find_equal(__parent, __k);
- __node_pointer __r = static_cast<__node_pointer>(__child);
+ auto [__parent, __child] = __tree_.__find_equal(__k);
+ __node_pointer __r = static_cast<__node_pointer>(__child);
if (__child == nullptr) {
__node_holder __h = __construct_node_with_key(__k);
__tree_.__insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__h.get()));
@@ -1444,8 +1443,7 @@ _Tp& map<_Key, _Tp, _Compare, _Allocator>::operator[](const key_type& __k) {
template <class _Key, class _Tp, class _Compare, class _Allocator>
_Tp& map<_Key, _Tp, _Compare, _Allocator>::at(const key_type& __k) {
- __parent_pointer __parent;
- __node_base_pointer& __child = __tree_.__find_equal(__parent, __k);
+ auto [_, __child] = __tree_.__find_equal(__k);
if (__child == nullptr)
std::__throw_out_of_range("map::at: key not found");
return static_cast<__node_pointer>(__child)->__get_value().second;
@@ -1453,8 +1451,7 @@ _Tp& map<_Key, _Tp, _Compare, _Allocator>::at(const key_type& __k) {
template <class _Key, class _Tp, class _Compare, class _Allocator>
const _Tp& map<_Key, _Tp, _Compare, _Allocator>::at(const key_type& __k) const {
- __parent_pointer __parent;
- __node_base_pointer __child = __tree_.__find_equal(__parent, __k);
+ auto [_, __child] = __tree_.__find_equal(__k);
if (__child == nullptr)
std::__throw_out_of_range("map::at: key not found");
return static_cast<__node_pointer>(__child)->__get_value().second;
|
ldionne
requested changes
Aug 28, 2025
aa226b2
to
cb3793a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.