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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 4 inline comments as done.
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:
> 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.
> 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 

No, it's not adding a brand new extension. libc++ has always supported this, regardless of the library's intention. That's how Hyrum's Law works.

Both Chrome OS and Android have been severely and adversely impacted by the changes made by D114920, which is why I'm trying to revert it. We were given no warning and no window of opportunity to change anything and have had the rug pulled out from under us. I am simply trying to unbreak the scores of packages that are using this as a result of this "accident", and would appreciate libc++ taking responsibility for the problems that D114920 has caused our teams.

> I'm not asking that you improve the overall test coverage for the distributions on other standard-mandated types 

That's exactly what's being asked here. You don't have the tests for `short`, `unsigned int`, etc., and in order for me to add tests for `unsigned char` and friends, I will need to do this very thing.

The lack of tests for supported code is indeed a shame, and it's the responsibility of libc++ developers to ensure that libc++ code is tested. I encourage libc++ contributors to take some time out to improve the tests for `binomial_int_distribution` and friends, but again, D125283 is a rollback of a patch that breaks downstream users who were not broken prior to this patch.


================
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:
> 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.
Sounds like I can close this thread out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125283