-
Notifications
You must be signed in to change notification settings - Fork 238
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
[compiler] consolidate BindingEnv interface #14475
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @patrick-schultz and the rest of your teammates on Graphite |
1a5d15b
to
8a060f9
Compare
8a060f9
to
f1a3628
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.
This is a really nice change and elegantly implemented. Thank you.
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.
Graphite fail
f1a3628
to
816819e
Compare
816819e
to
bf95f99
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.
laavely jubly
Follows up on #14475 by deleting the old implementation of `Bindings` in favor of the new one, which has now been checked by CI to not change the behavior.
As a step towards coralling the specification of the binding structure of the IR into one place, this rewrites
Bindings
to use only a single method of theGenericBindingEnv
interface,newBlock
, which therefore captures all possibilities of how a node can modify its parent's environment in a child. Later work refactors this to return an object encoding this modification, instead of returning a modified environment, which allows the caller complete flexibility in how to maintain an environment appropriately for their use case.This PR leaves in the old
Bindings
implementation, with an assertion that they agree. The PR stacked above this, #14495, deletes the old implementation. This way CI asserts that this refactoring hasn't changed any behavior.