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.

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

Support moving nodes and refactor to eliminate recursion #14

Merged
merged 11 commits into from
Aug 9, 2024

Conversation

serges147
Copy link
Collaborator

@serges147 serges147 commented Jul 29, 2024

  • Fix for issue Refactor to eliminate recursion #13
  • Added new postOrderTraverse method
  • Reduced a bit code duplication at const vs non-const traversal algorythms - now single traverseXxxOrderImpl covers these 2 almost identical cases.
  • Make nodes movable
  • Rename traversetraverseInOrder; added new traversePostOrder.

@serges147 serges147 self-assigned this Jul 29, 2024
c++/cavl.hpp Show resolved Hide resolved
Copy link
Owner

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

Looks good. I would reduce blank lines in the implementation to maintain style consistency.

c++/cavl.hpp Show resolved Hide resolved
@pavel-kirienko
Copy link
Owner

Please tag a new major release afterward.

@pavel-kirienko pavel-kirienko linked an issue Jul 31, 2024 that may be closed by this pull request
Copy link
Owner

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

  • I want to simplify the remove methods (both overloads) such that they don't accept the root pointer reference anymore, because this is already available in the node itself. I think this should be a trivial change. NB: search cannot be simplified in this fashion because it doesn't take an explicit node as a parameter and it may be invoked on an empty tree.

  • Consider adding root pointer invariant checks when the tree is mutated. We know that for any valid node, the root pointer pointer is never null.

  • There is a minor style nit I left an in-place comment about.

Please merge and release v3 afterward. This is a very useful change because it will also allow us to save memory in another project.

c++/cavl.hpp Outdated Show resolved Hide resolved
c++/cavl.hpp Show resolved Hide resolved
@pavel-kirienko pavel-kirienko changed the title Refactor to eliminate recursion Support moving nodes and refactor to eliminate recursion Aug 6, 2024
@pavel-kirienko
Copy link
Owner

But wait, there's more! We can't trivially move the tree class anymore:

/// Trees can be easily moved in constant time. This does not actually affect the tree itself, only this object.

We must traverse the entire tree updating the root pointer pointer in every node. That should be easy, can you please add it?

@serges147
Copy link
Collaborator Author

But wait, there's more! We can't trivially move the tree class anymore:

/// Trees can be easily moved in constant time. This does not actually affect the tree itself, only this object.

We must traverse the entire tree updating the root pointer pointer in every node. That should be easy, can you please add it?

Yes, you are right - I've missed this :(

serges147 and others added 2 commits August 9, 2024 14:11
* - Return back to 4x ptr node footprint
- Introduced new "origin" fake node at tree.
- Fixed tree movement - still constant time!

* minor fix

* delete not in use constructor
@serges147 serges147 enabled auto-merge (squash) August 9, 2024 11:44
@serges147 serges147 merged commit e48c349 into main Aug 9, 2024
14 checks passed
@serges147 serges147 deleted the sshirokov/issue_13_recursion branch August 9, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor to eliminate recursion
2 participants