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 #8635 #8636

Merged
merged 1 commit into from Apr 23, 2019

Conversation

@diml
Copy link
Member

commented Apr 23, 2019

Previously, when -allow-approx was passed and an input file wasn't parsing, the following would happen:

  • the parsing in read_parse_and_extract would fail
  • the error would be reported using report_err which would set error_occurred to true
  • read_parse_and_extract would call read_and_approximate because of -allow-approx
  • read_and_approximate would reset error_occurred to false so that the parsing failure wouldn't be treated as an error

However, this had the side-effect of clearing all other errors, which is not great. After this PR, the behaviour is as follow when a file doesn't parse:

  • the parsing in read_parse_and_extract fails
  • the error is printed but error_occurred is not modified
  • if -allow-approx was passed we call read_parse_and_extract, otherwise we set error_occurred to true

To make it harder for such bug to re-occur, I hid the error_occurred reference in a sub-module and only provided a way to set it to true.

@diml diml force-pushed the diml:ocamldep-error-handling branch from 2bf7f8d to 4ae4aaa Apr 23, 2019
@gasche
gasche approved these changes Apr 23, 2019
Copy link
Member

left a comment

The patch seems correct and it does fix a return-code bug. Good to go if the CI agrees.

@diml

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

CI just passed, merging

@diml diml merged commit 1f026fd into ocaml:trunk Apr 23, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.