-
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 how an instruction's used/blocked frames are calculated #73
Conversation
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.
Reasoning is sound, and the solution is clear. Small nitpicks and maybe some additional tests, but not blocking (heh).
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.
Nicely done! A few questions, but barring the failing tests, LGTM
All, | ||
|
||
/// Match all frames which shares any one of these names | ||
AnyOfNames(&'a [String]), |
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.
Why slices here? You're using vectors below (because of needing to put recursive ones on the heap), and if these get large enough, I imagine a set-based solution would be faster.
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.
Removing the lifetime stuff would probably simplify downstream code as well.
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.
Makes it a little simpler/cheaper to build the filter in the first place, without cloning. I originally had vecs everywhere, then slices, and then came to this and it came out cleanest. It is inconsistent though
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 suppose it makes sense if you see them all as slices but the Vec as just a Box<&[_]>
due to the type recursion
LGTM 🎉 |
Note: this fails the example cited in this issue; if the spec remains unchanged, the approach here will need to be modified. |
Closes #72
See issue for context.
Reworks the way that frame-blocking is computed for an instruction within the scope of a program. The previous way was to perform that work directly in
Program::get_used_frames
and then just pull those frames out of the program'sFrameSet
. This was brittle and harder to understand in the light of the Quil spec.This new approach is simpler in that it takes place in two steps:
FrameMatchCondition
is computed for an instruction, which is independent of the program in which that instruction lives.FrameSet
, which converts the "abstract" frame requirements of the instruction to the concrete frames available in the program.It also adds a new private method,
Instruction::parse
, for the convenience of the tests, so that single instructions can easily be used as fixtures.I considered using
rstest
for the table-driven test, but I'm not sure if we need that yet, and it will be easy to change over if we do.Notes for reviewers:
DELAY
's behavior is not fully specified in the spec, and so I kept its behavior consistent with how it was already enforced here. Thoughts welcome on that.include_blocking
is not enough information to distinguish all use cases to know the frames for an instruction. Currently, this allows for returning either used frames (those which the instruction will actively use, and for which it will take a dependency on instructions which earlier block it) or blocked frames (instruction may or may not use it, but it prevents other instructions from using it until it's done). Is there any other category of frames that we'd want to query for, or is this boolean argument enough?