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

comment-out R::pythag #826

Merged
merged 2 commits into from Mar 2, 2018

Conversation

Projects
None yet
2 participants
@eddelbuettel
Member

eddelbuettel commented Feb 28, 2018

Per email from BDR

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Mar 1, 2018

No comments by anyone? Should be harmless -- may just merge in a few ...

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Mar 1, 2018

We might (?) need a few more changes on our side:

inst/include/Rcpp/sugar/functions/complex.h
28:# define RCPP_HYPOT ::Rf_pythag

inst/include/Rcpp/sugar/undoRmath.h
110:#undef pythag

inst/include/Rcpp/Rmath.h
222:    inline double pythag(double a, double b)        { return ::Rf_pythag(a, b); }

I think the bit in undoRmath.h can stay, but we might want to tweak what we have in complex.h:

kevin@cdrv:~/r/pkg/Rcpp [rng-state]
$ ag -Q "RCPP_HYPOT"
inst/include/Rcpp/sugar/functions/complex.h
26:# define RCPP_HYPOT ::hypot
28:# define RCPP_HYPOT ::Rf_pythag
83:             y.r = ::log( RCPP_HYPOT( x.r, x.i ) );
90:         if( (mag = RCPP_HYPOT(z.r, z.i)) == 0.0)
144:    t1 = 0.5 * RCPP_HYPOT(x + 1, y);
145:    t2 = 0.5 * RCPP_HYPOT(x - 1, y);

If I understand correctly, R removed that function because C99 now defines ::hypot() and so R doesn't need to provide its own definition. (If I understand correctly, Rf_pythag() was R's own definition of ::hypot(), for platforms that didn't provide it back in the day)

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Mar 1, 2018

That said, if everything compiles and checks okay we can probably leave it as is?

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Mar 1, 2018

There is indeed a bit more. You first catch is good: RCPP_HYPOT is conditionally defined with a now-dead fallback. The undef is harmless. The inline we can just comment out as well.

The use of RCPP_HYPOT may need a second look as RCPP_HYPOT now has no fallback. Question is then ... would #ifdef HAVE_HYPOT ever not be true? And if so, shall we just emit a warning there?

man 3 hypot points to C99, POSIX.1-2001, POSIX.1-2008. Good enough?

Also:

edd@rob:~$ grep HAVE_HYPOT ~/svn/r-devel/config.status 
S["RMATH_HAVE_HYPOT"]="# define HAVE_HYPOT 1"
D["HAVE_HYPOT"]=" 1"
edd@rob:~$ 
@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Mar 1, 2018

The cleanest may just be to rewrite complex.h and do away with RCPP_HYPOT in favour of ::hypot. Let me pile that on later today.

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Mar 1, 2018

I think HAVE_HYPOT is effectively always true since (as you pointed out) it's defined by the C99 standard and I think R requires C99 or greater during compilation of R + extensions.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Mar 2, 2018

@kevinushey That was an excellent catch. I maded some changes, when you have moment, could you peruse?

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Mar 2, 2018

LGTM -- thanks for taking care of this!

@eddelbuettel eddelbuettel merged commit 06b1f62 into master Mar 2, 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

@eddelbuettel eddelbuettel deleted the feature/Rmath_header_cleanup branch Mar 8, 2018

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