[libcxx-commits] [PATCH] D114129: [libc++] Fix `uniform_int_distribution` for 128-bit result type

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

+1 it's a good idea to investigate the possibility of reusing `__countl_zero` and `__bit_log2`.



================
Comment at: libcxx/test/std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/int128.pass.cpp:35
+      const __int128_t n = d(e);
+      all_in_range = all_in_range && (n <= UINT64_MAX);
+    }
----------------
Mordante wrote:
> This feels flaky. If the tests happens to produce 100 random values below `UINT64_MAX` the test fails.
> I suggest to select a specific random number engine with a fixed seed for this test.
> Then you can add a stable test to see whether values in the wanted range are produced.
This isn't //literally// flaky, since `default_random_engine` will be fixed at compile time and so will its default seed — we're not using `random_device` or anything. I agree its definition might vary between libc++/libstdc++ and MSVC (I'm not sure if it does in practice; I'm pretty sure off the top of my head that it's `minstd_rand0` for both libc++ and libstdc++).
Note that engines' behaviors are defined by the standard, so `minstd_rand0{}()` has the same value on all implementations by definition. It's the //distributions// whose behaviors diverge across implementations (and unfortunately it's the //distribution// whose behavior we really care about here).

The chances of 100 numbers //all// being clustered in one 18446744073709551616th of the PRNG's range (or even one-tenth of its range, as on line 42) strikes me as actually-far-more-than-astronomically unlikely. So I don't see a problem here. But, at the same time, it wouldn't hurt to `s/default_random_engine/minstd_rand0/` just to remove one superfluous degree of freedom.


================
Comment at: libcxx/test/std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/int128.pass.cpp:51
+      assert(n >= a && n <= b);
+      all_in_range = all_in_range && (n >= INT64_MIN) && (n <= (INT64_MAX));
+    }
----------------
Mordante wrote:
> Why do we need this test? Isn't `assert(n >= a && n <= b);` enough to test?
I believe this illustrates my recent point about renaming this variable. :) We want to check that all the values //are// in the range `[a, b]`; but then we also want to check that //some// of the values are //not// in the narrower range `[INT64_MIN, INT64_MAX]` (because if they //all// happened to fall in that narrower range, that would likely indicate a truncation bug somewhere in libc++ code).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114129/new/

https://reviews.llvm.org/D114129