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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__random/binomial_distribution.h:28
+template <class _IntType = int>
+class _LIBCPP_TEMPLATE_VIS binomial_distribution {
 public:
----------------
cjdb wrote:
> 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`.
I think it's easier to forget about the fact that this code used to work by accident. With that in mind, what this patch effectively does is *add* a brand new extension (that we will document and support officially) for additional types in `binomial_distribution` & friends. What I'm saying is that this new extension needs to be tested, just like we test everything else in the library. Otherwise, we don't have a way to make sure that we support it properly, and that's one of the reason why we made sure that people would not use it by accident (by adding `static_assert`s).

I'm not asking that you improve the overall test coverage for the distributions on other standard-mandated types (although that would certainly be nice). Our testing of distributions is very light, and that's a shame. What I'm asking is to meet the bar that we have for all patches: that you test the new patterns that we did not officially support previously and that we will support going forward.


================
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);
----------------
manojgupta wrote:
> cjdb wrote:
> > 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.
> Shouldn't just checking for lowest bit ( val & 0x1)  work for bool uniform distribution?
I just re-read the implementation and I think I get it, and I think it works. It's actually pretty simple -- we'll end up generating a discrete uniform distribution on `unsigned char` between 0 and 1, so that maps directly to `false` and `true`. Somehow I had in mind that we were generating a distribution from `0` to `numeric_limits<unsigned char>::max()`, which would not have worked.


================
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);
----------------
cjdb wrote:
> 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>`.
This comment tied into my comment above about how the distribution is generated. If a distribution on `0, numeric_limits<unsigned char>::max()` was generated instead and the result type was `char` instead of `bool`, this test would pass even though it should not. So I think it'd be better to use `bool` here, if only to show that we're really getting a `bool` out of the distribution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125283