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 bug in GPR#1476 #1701

Merged
merged 2 commits into from Apr 9, 2018

Conversation

Projects
None yet
4 participants
@mshinwell
Copy link
Contributor

mshinwell commented Apr 6, 2018

#1476 contains a bug which causes random segfaults. The addition of the new code necessitated a root registration for the variable result, but this was not done.

The following reviewers are in the doghouse: @damiendoligez @lpw25 and @gasche .

@mshinwell mshinwell requested a review from damiendoligez Apr 6, 2018

@mshinwell mshinwell added this to the 4.07 milestone Apr 6, 2018

@gasche

gasche approved these changes Apr 6, 2018

Copy link
Member

gasche left a comment

The fix looks clearly correct to me. Why didn't we catch that? (I dream of a world where we can automate sanity checks of our C code.)

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 6, 2018

Minor thing, given that the GPR itself is recent, you could have just added this GPR number to its change entry (and your name in the credit list, "bugfix by ...") instead of doing your own entry. But the current approach is also fine.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

mshinwell commented Apr 6, 2018

The funny thing is that we thought the segfaults we were getting when building with trunk were all coming from #1660 ... but in fact there were two sources, this being the other!

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Apr 6, 2018

Oops, sorry about that, and thanks for the fix!

@damiendoligez
Copy link
Member

damiendoligez left a comment

The fix is clearly correct. Well done finding this!

@mshinwell mshinwell merged commit dc0dbd4 into ocaml:trunk Apr 9, 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

lpw25 added a commit to lpw25/ocaml that referenced this pull request May 22, 2018

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.