thomasp85 / farver

High Performance Colourspace Manipulation in R
https://farver.data-imaginist.com
Other
125 stars 14 forks source link

Colours cannot be encoded on s390x #16

Closed QuLogic closed 4 years ago

QuLogic commented 4 years ago

On s390x, trying to build farver 2.0.1, most (all?) tests that check colour encoding fail:

  == testthat results  ===========================================================
  [ OK: 64 | SKIPPED: 0 | WARNINGS: 0 | FAILED: 15 ]
  1. Failure: colours can be encoded (@test-encoding.R#7) 
  2. Failure: colours can be encoded (@test-encoding.R#8) 
  3. Failure: colours can be encoded (@test-encoding.R#9) 
  4. Failure: colours can be encoded (@test-encoding.R#10) 
  5. Failure: colours can be encoded (@test-encoding.R#11) 
  6. Failure: colours can be encoded (@test-encoding.R#12) 
  7. Failure: colours can be encoded (@test-encoding.R#13) 
  8. Failure: colours can be encoded (@test-encoding.R#14) 
  9. Failure: colours can be encoded (@test-encoding.R#15) 
  1. ...

As s390x is a big-endian architecture, I think doing things like this won't work.

thomasp85 commented 4 years ago

can I get you to test that this has been resolved by installing and checking the GitHub version?

QuLogic commented 4 years ago

Unfortunately not. Looking at the log, I just noticed this warning:

encode.cpp: In function 'int double2int(double)':
encode.cpp:18:33: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
   18 |   return reinterpret_cast<int&>(d);
      |                                 ^

and the line number indicates that the #if was not taken.

QuLogic commented 4 years ago

It looks like you can check WORDS_BIGENDIAN from the R header for this.

thomasp85 commented 4 years ago

yeah - was just looking at the same... can you test again

QuLogic commented 4 years ago

OK, we're good now; works on x86_64 and s390x (didn't want to tie up other stuff). Thanks for the quick fix.