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 build on big-endian machines. #81

Merged
merged 1 commit into from Sep 2, 2020
Merged

Conversation

@QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Aug 30, 2020

As the bytes are in the reverse order, the bitfields need to be reversed in order to produce the same special numbers.

Alternatively, the IVAL_MAX, IVAL_MIN, IVAL_NA could be changed, but I think it looks better this way in case you print out the values, as they seem more obviously special.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Aug 30, 2020

Thanks for that! We will take a good look.

What hardware platform are you on?

@QuLogic
Copy link
Contributor Author

@QuLogic QuLogic commented Aug 30, 2020

This is on Fedora s390x. Unfortunately, the build logs are gone from that attempt, but the result was:

Running the tests in 'tests/tinytest.R' failed.
Last 13 lines of output:
  ----- FAILED[data]: test_nanoival.R<503--503>
   call| expect_identical(is.unsorted(c(x, y, NA_nanoival_), na.rm = TRUE), 
   call| FALSE)
   diff| Expected FALSE, got TRUE
  ----- FAILED[data]: test_nanoival.R<1232--1232>
   call| expect_identical(ii[c(T, F, NA)], c(i1, NA_nanoival_))
   diff| Mean absolute Mod difference: 2.828427
  ----- FAILED[data]: test_nanoival.R<1249--1249>
   call| expect_identical(ii[c(1, NA)], c(i1, NA_nanoival_))
   diff| Mean absolute Mod difference: 2.828427
  ----- FAILED[data]: test_nanoival.R<1334--1334>
   call| expect_true(all(is.na(nn[1:3])))
   diff| Expected TRUE, got FALSE
  ----- FAILED[
  Execution halted
@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Sep 1, 2020

I just merged one smaller pending changeset, I may have one more -- I think I can commit back to your branch but I am not sure I force-push after rebasing. I may try, if it fails maybe I'll have to ask you to rebase. Hope that's ok.

@QuLogic
Copy link
Contributor Author

@QuLogic QuLogic commented Sep 1, 2020

I can rebase, but I think the option should be checked that you could do it.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Sep 1, 2020

Ok, lemme clean up once more and merge one more and then I can try.

We really appreciate your PR! @lsilvest wants to look into / think through one more aspect related to bit patterns and NAs. But I think this will go in and I am aiming for a new release in a few days.

@lsilvest
Copy link
Collaborator

@lsilvest lsilvest commented Sep 1, 2020

@QuLogic : I was a bit worried with the way we defined NA_nanoival_. I think endianness will not matter for nanotime and duration because they are based on integer64. nanoperiod should also be mostly OK I believe.

There should be some tests for the NAs (not sure if your changes passed R CMD check?), but otherwise, this ah hoc check should give us piece of mind: does is.na(NA_nanoival_) return TRUE on your machine?

Also, would you be able to test is.na(NA_nanoperiod_)?

Many thanks for all this!

@QuLogic QuLogic force-pushed the QuLogic:fix-be branch from 4748b26 to 266e185 Sep 1, 2020
@QuLogic
Copy link
Contributor Author

@QuLogic QuLogic commented Sep 1, 2020

Before:

> is.na(NA_nanoival_)
[1] FALSE
> is.na(NA_nanoperiod_)
[1] TRUE

On this branch:

> is.na(NA_nanoival_)
[1] TRUE
> is.na(NA_nanoperiod_)
[1] TRUE

Actually, I noticed one more constructor's reordering warnings, so I pushed an update that should fix that, and rebased.

As the bytes are in the reverse order, the bitfields need to be reversed
in order to produce the same special numbers.
@eddelbuettel eddelbuettel force-pushed the QuLogic:fix-be branch from 266e185 to 9dd161d Sep 1, 2020
@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Sep 1, 2020

@QuLogic You were correct, and I was too pessimistic. The toggle does indeed allow me to force-push back, so I rebased your PR against our main branch having merged two pending sets of changes.

@lsilvest How do you feel about NA etc now? Ok to merge this and release an updated 0.3.2 to CRAN? We would have a few more days for tests so it doesn't have to happen to day or tomorrow...

@lsilvest
Copy link
Collaborator

@lsilvest lsilvest commented Sep 1, 2020

Yes, absolutely, good to merge!

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Sep 1, 2020

@QuLogic And just to double-check, and to make sure I didn;t inadvertendly trample over anythin with the force push: all your changes are still there, right? (Behaves fine on r-release and r-devel for me, but that is on x86_64.)

@QuLogic
Copy link
Contributor Author

@QuLogic QuLogic commented Sep 2, 2020

Yes, I did not re-compile it again, but looking at the diffs it appears the same still.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Sep 2, 2020

Thanks--I was a little worried I might not have pulled a second time before pushing.

PS But come to think to about it I did only create my local branch off your remote branch after your 2nd commit so I was overly worried. We should be just fine. Test builds on Windows now finally see the (needed and checked for) updated RcppCCTZ so we're all set now).

@eddelbuettel eddelbuettel merged commit e9cbbc2 into eddelbuettel:master Sep 2, 2020
3 checks passed
3 checks passed
codecov/patch Codecov Report
Details
codecov/project Codecov Report
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Sep 2, 2020

It's all ready and tests fine but the CRAN Windows machine with r-devel still has RcppCCTZ so I have to wait on that.

@QuLogic QuLogic deleted the QuLogic:fix-be branch Sep 2, 2020
@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Sep 4, 2020

This is now on CRAN -- thanks again!

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.