-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Clang build error wrt isnan and isinf #863
Comments
Verified that this one change does make it build on FreeBSD. PR coming shortly. |
Fixed in 1.6.3 |
I am still getting this error with node v8.
npm ERR! code ELIFECYCLE npm ERR! A complete log of this run can be found in: |
This was fixed in 1.6.3. You need to use a newer version (or a newer version of resemble that uses a newer version of canvas). |
Thanks |
canvas 1.5.0, but the offending code is also in master.
Happens due to this line:
https://github.com/Automattic/node-canvas/blob/master/src/CanvasRenderingContext2d.cc#L29
So all
isnan
references in the file are expanded tostd::isnan
, if globalisnan
is not a macro. But it is not a macro on Clang, it's a plain function. And then, because the file includes math.h rather than cmath, it fails to findisnan
instd
(math.h only guarantees that the things that it defines are in the global namespace; that it happens to also bring them intostd
on some implementations is a non-standard quirk of those implementations). Same thing is happening withisinf
.This is Clang 3.8.0 on FreeBSD, but I believe it'll repro on any platform, because the included header is from the Clang standard library. You can see what their math.h and cmath look like here:
https://github.com/llvm-mirror/libcxx/blob/master/include/math.h
https://github.com/llvm-mirror/libcxx/blob/master/include/cmath
The fix should be to simply
#include <cmath>
instead of<math.h>
, or else drop thestd::
and just useisinf
/isnan
from global namespace directly.The text was updated successfully, but these errors were encountered: