ircmaxell/RandomLib — pull request #23 by banks (2014-04-10 09:38:30+00:00)
Title: Fix issue #22

The code fix looks good. I did not pull this exact branch locally to test it but I have experimented with identical code myself and tested OK.

The test you added raises some questions but I'll leave it to you to decide on your priorities there.

Not sure why I can't comment inline on the diff below but my thoughts:

* "Only test on 64 bit platforms" the bug would presumably manifest in just the same way on 32 bit platforms only with a range of 31 bits. Certainly as it is now with left shift the same would happen where `1 << $bits` would become the largest negative number possible and -1 would cause it to become a float. I'm not sure off top of my head if that float might retain enough precision to avoid the bug but I doubt it since it is also only 4 bytes. I suppose to be safe there should be a second special case where `$bits == 31 && PHP_INT_SIZE == 4`. Then your test should work identically on either platform (assuming you swap the expected number based on int size). I don't have a 32bit install to verify this on to hand however. Or if you don't care about 32bit then that's fine but it's a pretty big gotcha not to document upfront.
* `$this->assertEquals(506381209866536711, $this->generator->generateInt(0, PHP_INT_MAX));` My first response was surely this test never passes but I see you are mocking the generator to have a totally predictable 'random' source in these tests. That means my bug report was slightly misleading since I assumed I would get different result from each call to `generateInt` and got all 0. That's not really very significant though - the actual behaviour due to the bug was wrong and is fixed by this anyway.

===Summary===

LGTM although the test skip highlights that this will not fix the equivalent issue for 32 bit PHP.