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

Refactor inlining/specialisation (flambda) #1663

Merged
merged 5 commits into from Apr 6, 2018

Conversation

Projects
None yet
5 participants
@xclerc
Copy link
Contributor

xclerc commented Mar 19, 2018

This PR rewrites the Inlining_transforms.inline_by_copying_function_declaration
function to make it easier to understand, and adds the information about free variables
of sets of closures to exported infos.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

xclerc commented Mar 19, 2018

The refactoring also fixes a bug (triggered by the file added to the test suite).

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 20, 2018

@xclerc

This comment has been minimized.

Copy link
Contributor Author

xclerc commented Mar 20, 2018

This PR has only been reviewed by @lpw25.
We hence need another review, typically from @chambart.

@chambart

This comment has been minimized.

Copy link
Contributor

chambart commented Mar 30, 2018

I'm overall ok with this patch. I won't approve yet since I need a bit more time to go through some details, but I think nothing bad will lurk there.

The only thing that doesn't convince me right now is the record for binding variables. This feels a bit like uninitialized variables to me. For instance, if some refactoring forget to initialize some field or some part becomes dead, the compiler won't be able to tell that to us. I understand the syntactic/documentation benefit here, I just don't know if we can do better.

@lpw25

This comment has been minimized.

Copy link
Contributor

lpw25 commented Apr 4, 2018

The only thing that doesn't convince me right now is the record for binding variables.

Do you mean the let_bindings field of the state type? If so, then that seems to be identical to the free_vars_for_lets variable in the old code, so I don't think the risk of not updating it is any different in the new code.

@chambart

This comment has been minimized.

Copy link
Contributor

chambart commented Apr 6, 2018

That's ok. It's only some nitpicking. I don't have a better way to write that part. It just feel that we are losing some compiler help.

Anyway I'm ok.

@chambart chambart added the approved label Apr 6, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 6, 2018

(I'm going to merge this now and keep an eye on the CI on the trunk branch, since any further failures due to the latest rebase are unlikely.)

@mshinwell mshinwell merged commit 7a9723b into ocaml:trunk Apr 6, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.