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 PR#7414 for local modules #1778

Merged
merged 2 commits into from May 17, 2018

Conversation

Projects
None yet
4 participants
@lpw25
Copy link
Contributor

lpw25 commented May 9, 2018

#929 fixed the soundness bug from PR#7414. However, it only fixed it for modules in structures not local modules. This PR extends it to cover local modules and adds a test case.

@mshinwell mshinwell added the bug label May 9, 2018

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented May 14, 2018

@garrigue Sorry to ask you for another review whilst I still haven't got around to reviewing your PR, but could you have a quick look at this one. It's a soundness bug with a one line fix, so I'd like to get it in 4.07 if possible.

@garrigue
Copy link
Contributor

garrigue left a comment

This is indeed a soundness bug, and this one-liner fixes it.
There is no guarantee that this will not break existing programs (this the price of soundness!), but it clearly cannot introduce any new soundness bug.

@garrigue

This comment has been minimized.

Copy link
Contributor

garrigue commented May 15, 2018

By the way, since this is an incompatible change, you need an entry in Changes.

@damiendoligez damiendoligez added this to the 4.07 milestone May 16, 2018

@lpw25 lpw25 merged commit f4fbdab into ocaml:trunk May 17, 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
@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented May 17, 2018

@lpw25 have you cherry-picked to 4.07?

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented May 17, 2018

I'm just testing it now before I push.

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented May 17, 2018

Cherry-picked to 4.07 (1dd8c8d..4ce7016)

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.