search

haskell / random

Random number library
Other
53 stars 50 forks source link

Improve uniform ShortByteString (fixup) #118

Closed lehins closed 3 years ago

lehins commented 3 years ago

This PR attempts to fix a bug that sneaked in the #116 that affects Big Endian machines

I've tested it on 32/64bit Intel and 32bit armv7 machines, however both of these are little endian and I still have not figured out a way to test big endian code. @juhp, maybe you can help us out once more and run this PR on a s390x again? Thanks to your feedback in https://github.com/haskell/random/pull/116#issuecomment-911600622 I am pretty confident that this change will do the trick.

juhp commented 3 years ago

Hey @lehins I tested 53e3b63: afraid there seems to be another regression though:

Linking /tmp/petersen/ghc-random/random/dist-newstyle/build/s390x-linux/ghc-8.8.4/random-1.2.1/t/doctests/build/doctests/doctests ...
Running 1 test suites...
Test suite doctests: RUNNING...
src/System/Random.hs:199: failure in expression `unpack . fst . genByteString 10 $ pureGen'
expected: [51,123,251,37,49,167,90,109,1,4]
 but got: [109,90,167,49,37,251,123,51,1,4]
           ^

Examples: 134  Tried: 134  Errors: 0  Failures: 1
Test suite doctests: FAIL
:
:
: 
    genByteString/ShortByteString consistency:  FAIL
      test/Spec.hs:118:
      expected: [78,232,117,189,13,237,63,84]
       but got: [84,63,237,13,189,117,232,78]
lehins commented 3 years ago

Turned out to be a bug in GHC#20338. See this https://github.com/haskell/random/pull/116#issuecomment-913854038

@juhp If you don't mind, could you please run the test suite one last time on this PR and I promise I won't bug you again ;)

juhp commented 3 years ago

cabal test passes for fbcbaf6 on Fedora 34 s390x :+1:

FYI there was one build warning:

src/System/Random/Internal.hs:358:1: warning: [-Wunused-top-binds]
    Defined but not used: ‘writeWord32’
lehins commented 3 years ago

@juhp Awesome!!! Thank you very much!!!!!!

FYI there was one build warning

That one I can fix without re-running the tests. Thanks for pointing it out!