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

Fix "open struct .. end" on clambda backend #8944

Merged
merged 3 commits into from Sep 17, 2019

Conversation

@trefis
Copy link
Contributor

commented Sep 16, 2019

This has been broken since the introduction of the feature (so, 4.08).

@Octachron: You might want to consider this for 4.09.

@trefis trefis force-pushed the trefis:open-struct-clambda branch from fa8a120 to f861766 Sep 16, 2019
@Octachron

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

An one line miscompilation fix does seem like a valid candidate for 4.09 .

@lpw25
lpw25 approved these changes Sep 16, 2019
Copy link
Contributor

left a comment

LGTM

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

@trefis, would you mind moving the change entry to the 4.09 section?

Also by curiosity, if I follow correctly, the problem with the previous code was that it was breaking the invariant that access to previously defined global fields should access the field by position rather than by name. Am I right?

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Yeah, if you look at this line:

Lsequence(lam,

you can see where this invariant is relied upon. lam has been made by compiling the items of a structure and defined_idents str.str_items contains identifiers for those items. Those identifiers are out of scope, so it is relying on the subst to replace all of them with projections instead.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Thanks for the explanation.

@objmagic

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Thanks for the explanation and thanks again for fixing it... @trefis

@trefis trefis force-pushed the trefis:open-struct-clambda branch from f861766 to 49a9e33 Sep 17, 2019
@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

@Octachron: done.

@trefis trefis merged commit f56b59d into ocaml:trunk Sep 17, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
trefis added a commit that referenced this pull request Sep 17, 2019
@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

Cherry-picked on 4.09.

@trefis trefis deleted the trefis:open-struct-clambda branch Sep 17, 2019
lpw25 added a commit to janestreet/ocaml that referenced this pull request Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.