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 AIX compilation issue with NAN and INFINITY #3043

Merged
merged 2 commits into from Sep 12, 2018
Merged

Conversation

@ayappanec
Copy link
Contributor

@ayappanec ayappanec commented Sep 12, 2018

We have recently ported R to AIX .
https://www.ibm.com/developerworks/aix/library/aix-toolbox/alpha.html#R
When trying to install data.table, there is a compilation issue with NAN and INFINITY.

fread.c:79:28: error: initializer element is not constant
static const double NAND = (double)NAN;
^
fread.c:80:28: error: initializer element is not constant
static const double INFD = (double)INFINITY;
^
With gcc -E option , we see this

static const unsigned int _SINFINITY = 0x7f800000;
static const unsigned int _SQNAN = 0x7fc00000;

static const double NAND = (double)
79 "fread.c" 3 4
(*((float )(&_SQNAN)))
79 "fread.c"
;
static const double INFD = (double)
80 "fread.c" 3 4
(
((float *)(&_SINFINITY)))
80 "fread.c"

So they don't seem to qualify as constant literals hence the compilation issue.
I have tested the fix in Linux as well.

@codecov
Copy link

@codecov codecov bot commented Sep 12, 2018

Codecov Report

Merging #3043 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3043      +/-   ##
==========================================
+ Coverage   90.87%   90.87%   +<.01%     
==========================================
  Files          61       61              
  Lines       11734    11740       +6     
==========================================
+ Hits        10663    10669       +6     
  Misses       1071     1071
Impacted Files Coverage Δ
src/fread.c 97.96% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23de27c...abbd00f. Read the comment docs.

@jangorecki
Copy link
Member

@jangorecki jangorecki commented Sep 12, 2018

Hi, thanks for catching that, appreciate your PR.
Would be even better if you could:

  • add entry in NEWS
  • put comment next to init() declaration referring issue/PR number

Also note that we have Contributing wiki which describes how to make PR.
You have good timing, we are preparing 1.11.6 release. Next 1.12.0 will probably takes some time, so we should include this for 1.11.6.

@jangorecki jangorecki added this to the 1.11.6 milestone Sep 12, 2018
@ayappanec
Copy link
Contributor Author

@ayappanec ayappanec commented Sep 12, 2018

Thanks. I have updated the PR as per your info.

Copy link
Member

@jangorecki jangorecki left a comment

LGTM

@jangorecki jangorecki requested a review from mattdowle Sep 12, 2018
@mattdowle mattdowle merged commit 5049e56 into Rdatatable:master Sep 12, 2018
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 90.87%)
Details
codecov/project 90.87% (+<.01%) compared to 23de27c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.