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

Feature/complete pr799 #800

Merged
merged 3 commits into from Jan 14, 2018

Conversation

Projects
None yet
3 participants
@eddelbuettel
Member

eddelbuettel commented Jan 14, 2018

No description provided.

@krlmlr

This comment has been minimized.

Contributor

krlmlr commented Jan 14, 2018

I'd prefer std::isnan() because it's faster by another x1.5, but this also comes with a slight risk. What do you think?

@eddelbuettel eddelbuettel referenced this pull request Jan 14, 2018

Closed

Simplify is_na() #799

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Jan 14, 2018

Let's review this in the next cycle. I have much more profound problems with this 0.12.15 candidate now.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Jan 14, 2018

Also, R has NA and NaN, the standard only has the latter so I prefer R in general for this.

@eddelbuettel eddelbuettel merged commit 7d063b4 into master Jan 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@krlmlr

This comment has been minimized.

Contributor

krlmlr commented Jan 14, 2018

R's NA is a special NaN, always has been, and probably will be forever. But I agree it's safer to use R_cppisnan().

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Jan 14, 2018

R-exts has this to say about std::isnan():

Functions/macros such as isnan, isinf and isfinite are not required by C++98: where compilers support them they may be only in the std namespace or only in the main namespace. There is no way to make use of these functions which works with all C++ compilers currently in use on R platforms: use R’s versions such as ISNAN and R_FINITE instead.

which is a long-winded way of saying, "if you use std::isnan(), CRAN may yell at you later when it breaks on Solaris". So if we did switch to std::isnan() we should guard that usage based on whether gcc / clang is being used as the compiler.

@krlmlr

This comment has been minimized.

Contributor

krlmlr commented Jan 14, 2018

Thanks. I missed the "C++11" requirement for std::isnan(): http://en.cppreference.com/w/c/numeric/math/isnan. Let's stick with the API.

@eddelbuettel eddelbuettel deleted the feature/completePR799 branch Jan 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment