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 compilation of catches with multiple handlers #1973

Merged
merged 2 commits into from Feb 21, 2019

Conversation

@lthls
Copy link
Contributor

commented Aug 7, 2018

The Cmm and Mach representations have recently been updated to allow static catch constructions to contain more than one handler. There is currently no code that takes advantage of this feature, and as a result it is not well tested.

This pull request fixes a bug in the transformation of these multiple handlers to linear code.

The bug cannot be triggered from OCaml code currently, but the Cmm parser included in the testsuite already supported the feature, so I added a test program that would produce wrong code without this patch.
Unfortunately, I don't know how to ask ocamltest to run the actual produced code, let alone check its output, so the test would pass even without the fix.
If someone knows how to check the produced code's output, directions or patches are welcome.

Copy link
Contributor

left a comment

This is an obvious fix. The code is ok and comes with a test. I'm all for it. This is not urgent as the situation generating this doesn't happen in the current compiler, but this is required by future optimizations.

Changes Outdated
@@ -314,6 +314,9 @@ Working version
- GPR#1970: fix order of floatting documentation comments in classes
(Hugo Heuzard, review by Nicolás Ojeda Bär)

- GPR#1973: fix compilation of catches with multiple handlers
(Vincent Laviron)

This comment has been minimized.

Copy link
@chambart

chambart Feb 19, 2019

Contributor

This should move to the trunk section of the changelog

This comment has been minimized.

Copy link
@lthls

lthls Feb 19, 2019

Author Contributor

Fixed

@lthls lthls force-pushed the lthls:multihandlers branch from e9f6ed8 to 08fa9a5 Feb 19, 2019
@chambart chambart merged commit b99e54f into ocaml:trunk Feb 21, 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
@damiendoligez

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@shindere for the question about the tests (is there a way to run the code of a cmm test?)

@shindere

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 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.