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.

Sign up for GitHub

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

Jump to bottom

Fix exception when splitting F_sequences. #3199

Merged
merged 5 commits into from Aug 20, 2019
Merged

Fix exception when splitting F_sequences. #3199

merged 5 commits into from Aug 20, 2019

Conversation

enolan
Copy link
Contributor

@enolan enolan commented Aug 14, 2019

Fixes #3143.

  • Changed split predicate from > to >= to match paper
  • Fixed up other things to match
  • Improved finger tree tests
  • Fix handling of measures when splitting a subtree of Nodes and
    the split point is in the left finger.
  • Added a transaction pool test for maintaining the max size.
    This would not have caught the bug, but is nice to have.

To review this you should run git diff locally against the commit before I replaced the finger trees with lists.

@enolan
Fix exception when splitting F_sequences.
0dc35b6 
Fixes #3143.

* Changed split predicate from > to >= to match paper
* Fixed up other things to match
* Improved finger tree tests
* Fix handling of measures when splitting a subtree of Nodes and
  the split point is in the left finger.
* Added a transaction pool test for maintaining the max size.
  This would not have caught the bug, but is nice to have.
nholland94
nholland94 approved these changes Aug 16, 2019
View reviewed changes
Copy link
Member

@nholland94 nholland94 left a comment

Choose a reason for hiding this comment

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

I am not comfortable reviewing the finger tree code in depth (perhaps @bkase or @imeckler can provide a review for that), but the other details look good, so I'm approving to unblock.

@enolan enolan added ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR and removed ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR labels Aug 20, 2019
bkase
bkase approved these changes Aug 20, 2019
View reviewed changes
Copy link
Member

@bkase bkase left a comment

Choose a reason for hiding this comment

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

Finger tree stuff looks good

@enolan enolan added the ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR label Aug 20, 2019
@mergify mergify bot merged commit 836286c into develop Aug 20, 2019
@mergify mergify bot deleted the fix/finger-tree-split-exn branch August 20, 2019 21:53
@enolan enolan mentioned this pull request Aug 21, 2019
Closed
@enolan enolan mentioned this pull request Sep 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkase bkase bkase approved these changes

@nholland94 nholland94 nholland94 approved these changes

Assignees
No one assigned
Labels
ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CRASH: (monitor.ml.Error (Failure "Digit.split index out of bounds")
3 participants
@enolan @bkase @nholland94