[libcxx-commits] [PATCH] D125283: reverts "[libc++] Explicitly reject `uniform_int_distribution<bool>` and `<char>`."

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D125283#3535124 <https://reviews.llvm.org/D125283#3535124>, @EricWF wrote:

> In D125283#3535047 <https://reviews.llvm.org/D125283#3535047>, @ldionne wrote:
>
>> If you can add tests to show that distributions work with these types, I'd be OK with that. We still have until June 8th to cherry-pick to LLVM 14, so we could even backport to LLVM 14 and avoid breaking users for just one release (which would be very annoying indeed).
>
> Given the poor state of testing for the current random implementation, and given that writing really really high quality tests to prove the disctributions is hard, can you say what level of testing you would find sufficient?

Whatever level of testing we have right now for other non-`char` non-`bool` types. It seems that this is what @cjdb is doing here, so that sounds reasonable to me (except we are missing tests for a couple distributions in this patch as commented).



================
Comment at: libcxx/include/__random/binomial_distribution.h:28
+template <class _IntType = int>
+class _LIBCPP_TEMPLATE_VIS binomial_distribution {
 public:
----------------
Since we are removing the restriction for these distributions too, we should also be adding tests for their behavior.


================
Comment at: libcxx/include/__random/uniform_int_distribution.h:242-243
     static_assert(__libcpp_random_is_valid_urng<_URNG>::value, "");
-    typedef typename conditional<sizeof(result_type) <= sizeof(uint32_t), uint32_t,
-                                 typename make_unsigned<result_type>::type>::type _UIntType;
+    using _UIntType =
+        _If<sizeof(result_type) <= sizeof(uint32_t), uint32_t, typename __unsigned_equivalent<result_type>::type>;
     const _UIntType _Rp = _UIntType(__p.b()) - _UIntType(__p.a()) + _UIntType(1);
----------------
I don't think I understand how this works for `bool`. Since anything non-`0` will end up being considered `true` and only `0` is `false`, how do we achieve a uniform distribution by casting a uniformly-distributed `unsigned char` back to `bool`? Am I misunderstanding something?


================
Comment at: libcxx/test/std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/eval.pass.cpp:114-115
+  // We use 100x more values than non-bool tests to secure >99.9% confidence.
+  std::vector<signed char> values(1000000);
+  for (signed char& i : values) {
+    i = dist(gen);
----------------
Should that type be `bool`? You're getting `bool` out of `dist(gen)`, aren't you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125283