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

Cleanup around tree #1155

Merged
merged 11 commits into from
Dec 19, 2022
Merged

Cleanup around tree #1155

merged 11 commits into from
Dec 19, 2022

Conversation

raphaelrobert
Copy link
Member

This PR cleans up code around the ratcheting tree to correct previous design choices and make the transition to a full tree easier:

  • Limits valid platforms to 32, 64, and 128 bits of pointer width
  • Avoids unnecessary LibraryErrors in the binary tree
  • Improves how leaves are returned, making the public MlsGroup.members() a nice iterator that's no longer a Result
  • Improves merges, making MlsGroup.merge_staged_commit() LibraryError-free
  • Deletes now unnecessary code

The PR is split into commits that can be reviewed separately. Each commit contains a small set of code changes and the corresponding churn.

Note that this is just a partial cleanup and that more needs to be done.

@raphaelrobert raphaelrobert self-assigned this Dec 18, 2022
@raphaelrobert raphaelrobert requested a review from a team as a code owner December 18, 2022 17:31
Copy link
Member

@kkohbrok kkohbrok left a comment

Choose a reason for hiding this comment

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

I can't remember why we didn't just use config flags to restrict the architecture to begin with. I only remember that we discussed it at length and that I spent quite a bit of time to implement the current solution.

That being said, I think the solution in this PR is quite a bit more elegant than using library errors.


let mut combined = Vec::new();

loop {
Copy link
Member

Choose a reason for hiding this comment

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

I think getting rid of Itertools is nice, but loop is a bit dangerous. I seem to remember that we decided against using it in another instance due to the potential for an infinite loop, but I can't remember the specifics. This one seems pretty straight-forward due to how next is used in each iteration indicating that the loop must terminate at some point, so I don't have a strong opinion here.

@raphaelrobert raphaelrobert merged commit 9430ab8 into main Dec 19, 2022
@raphaelrobert raphaelrobert deleted the raphael/reduce_tree_errors branch December 19, 2022 15:44
@raphaelrobert raphaelrobert mentioned this pull request Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants