-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
data structure: implement pairing heap without use of shared_ptr #1713
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sanity checks are not running through and you need to rebase again please.
void walk_tree(const element_t &root, | ||
const std::function<void(const element_t &)> &func) const { | ||
func(root); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I didn't thought of that we have top do this for the deletion.
It's probably not a good idea to change the function like this because it effectively reverses the order of walking the tree (root
will now be the last node that func
is applied to). I guess the best solution then is to leave this function as-is and not use iter_all
.
Instead, you can use the code you have written here in clear()
but replace func(root)
with delete root
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding a reverse argument to walk tree? (see changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that is also a nice option.
I pushed a commit to your branch that additionally makes the reversing a compile-time option, making it faster to execute.
c3ed21a
to
fd7fd01
Compare
4b1b503
to
5a0c3ec
Compare
Looks like the amount of calls in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to merge. Well done!
Continuation of #1707 which was accidentally closed when I deleted my fork of openage