-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix: frame dependency calculation #76
Conversation
fd4785c
to
a2acfb9
Compare
a2acfb9
to
11a8e97
Compare
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.
Some requests for shorter code 🏌️, a suggestion about changing the return type of a function, but otherwise LGTM
src/program/graph.rs
Outdated
/// | ||
/// If the frame is currently blocked by other nodes, it will add itself to the list of blockers, | ||
/// much like a reader in a read-write lock. | ||
pub fn register_blocker(&mut self, node: ScheduledGraphNode) -> Option<ScheduledGraphNode> { |
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 can't think of a better name, so maybe ignore this, but I think a more suggestive function name that says something about returning self.using
might be helpful.
I suppose I have a similar (-ly unhelpful) note for register_user
's naming, too.
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.
Changed - what do you think?
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.
👍
Note: this fails the example cited in this issue; if the spec remains unchanged, this will have to be modified. |
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.
Nice. Took me a hot minute to understand the logic, but that might be because I've been awake since 5AM. Only question is: do the PreviousNodes
methods need to be public?
🎉 This PR is included in version 0.11.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Another fix, back to back with #73.
Closes #75
This takes the approach of tracking the instructions which use and block a frame separately, solving the problems described in #75. It's more complex, but should still remain readable and maintainable. This PR includes a couple more snapshot test cases to illustrate the differences and prevent regression.