-
Notifications
You must be signed in to change notification settings - Fork 79
Move fix for clang bug to C file. #758
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
Conversation
|
@petrelharp I guess this should be pulled into SLiM once this PR is approved, so I'm tagging you to put it on your radar. :-> |
17011db to
8919cb9
Compare
|
Ended up having to take a different approach here because we used isnan etc in the test suite as well. Same basic outcome though. Thanks for taking a look @bhaller. The 0.99.4 version should be tagged in the next day or so, so hopefully we can sync up on that for SLiM. |
Codecov Report
@@ Coverage Diff @@
## master #758 +/- ##
=======================================
Coverage 87.69% 87.69%
=======================================
Files 24 24
Lines 19388 19388
Branches 3640 3640
=======================================
Hits 17002 17002
Misses 1297 1297
Partials 1089 1089
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
This should be fine. |
|
@benjeffery, this look good to you? |
benjeffery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
8919cb9 to
5806861
Compare
This moves the workaround for the clang bug (#721) from the header file to a C file, to address very good points made by @bhaller. The fix is now local to C code, so we don't need to check for C++ any more, and library users will no longer have this workaround foisted on them unwittingly.
@molpopgen, do you see any problems with doing this?