Tree configuration #138
Open
Tree configuration #138
Conversation
This file contains 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
We don't need an interface (yet), since there is only one implementation. A client (user) of the package can easily add its own interface as necessary.
This makes the test data specify the label: number to make it more readable; it is hard to map the four numbers to the field name of original createTreeTest type.
We only need to store the IDs once; the index into the ID slice represents that ID's position in the tree. I think it will be faster for small trees to do this approach; we may consider to add a benchmark to find the breaking point where it is faster to use a map for this. If a map is needed for larger trees for speed, we could consider to revert this change, or allocate the map only if the tree is actually large.
meling
requested changes
Dec 22, 2024
trees/treeconfig.go
Outdated
| } | ||
|
|
||
| // GetHeight returns the height of the replica | ||
| func (t *Tree) GetHeight() int { |
There was a problem hiding this comment.
Do we need to export this? That is, will we use it in the code outside the tree package?
There was a problem hiding this comment.
This was useful to me for timer computation
trees/treeconfig.go
Outdated
| } | ||
|
|
||
| // GetSubTreeNodes returns all the nodes of its subtree. | ||
| func (t *Tree) GetSubTreeNodes() []hotstuff.ID { |
There was a problem hiding this comment.
Let's rename it to SubTree().
// SubTree returns the subtree of the tree's replica.
trees/treeconfig.go
Outdated
| queue = append(queue, children...) | ||
| subTreeNodes = append(subTreeNodes, children...) | ||
| if len(children) == 0 { | ||
| return subTreeNodes |
There was a problem hiding this comment.
This will also be empty slice, if len(children) == 0
There was a problem hiding this comment.
no children means, it is a leaf node hence empty subtree
trees/treeconfig.go
Outdated
| if len(children) == 0 { | ||
| return subTreeNodes | ||
| } | ||
| for len(queue) > 0 { |
There was a problem hiding this comment.
I'm a bit confused by the logic. I need to dig deeper. But perhaps it would make sense to use the std lib slices package for some of these things?
Maybe you could add a comment or two to explain the idea.
meling
reviewed
Dec 22, 2024
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.
create trees from position slice