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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/__random/binomial_distribution.h:28
+template <class _IntType = int>
+class _LIBCPP_TEMPLATE_VIS binomial_distribution {
 public:
----------------
ldionne wrote:
> Since we are removing the restriction for these distributions too, we should also be adding tests for their behavior.
I'd do so if the tests checked `short`, etc. were valid, but I don't think it's my responsibility to fill out the tests when they don't check all standard-required patterns. Each of the things I haven't touched only test `int` and one of `long` or `long long`.


================
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);
----------------
ldionne wrote:
> 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?
Good question, I don't have an answer. I can increase my sample size to boost confidence that the distribution is roughly uniform, if you'd like.


================
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);
----------------
ldionne wrote:
> Should that type be `bool`? You're getting `bool` out of `dist(gen)`, aren't you?
It can be `bool`, but it doesn't need to be. I didn't want to get yelled at for using the almost universally-hated `vector<bool>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125283