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

[compiler] consolidate BindingEnv interface #14475

Merged
merged 1 commit into from
May 1, 2024

Conversation

patrick-schultz
Copy link
Collaborator

@patrick-schultz patrick-schultz commented Apr 16, 2024

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 the GenericBindingEnv 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.

Copy link
Collaborator Author

patrick-schultz commented Apr 16, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @patrick-schultz and the rest of your teammates on Graphite Graphite

@patrick-schultz patrick-schultz force-pushed the ps-04-16-use_declarative_encoding_in_Binds branch 7 times, most recently from 1a5d15b to 8a060f9 Compare April 22, 2024 20:13
@patrick-schultz patrick-schultz force-pushed the ps-04-16-use_declarative_encoding_in_Binds branch from 8a060f9 to f1a3628 Compare April 23, 2024 16:50
@patrick-schultz patrick-schultz marked this pull request as ready for review April 24, 2024 14:13
@patrick-schultz patrick-schultz changed the title use declarative encoding in Binds [compiler] consolidate BindingEnv interface Apr 24, 2024
Copy link
Collaborator

@ehigham ehigham left a 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.

hail/src/main/scala/is/hail/expr/ir/Binds.scala Outdated Show resolved Hide resolved
hail/src/main/scala/is/hail/expr/ir/Binds.scala Outdated Show resolved Hide resolved
Copy link
Collaborator

@ehigham ehigham left a comment

Choose a reason for hiding this comment

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

Graphite fail

Copy link
Collaborator

@ehigham ehigham left a comment

Choose a reason for hiding this comment

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

laavely jubly

@hail-ci-robot hail-ci-robot merged commit 3f2e4d5 into main May 1, 2024
2 checks passed
@hail-ci-robot hail-ci-robot deleted the ps-04-16-use_declarative_encoding_in_Binds branch May 1, 2024 18:20
hail-ci-robot pushed a commit that referenced this pull request May 1, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants