[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.

In D114129#3165399 <https://reviews.llvm.org/D114129#3165399>, @leonardchan wrote:

> Hi. I think the build issue we're seeing (https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-debug-subbuild/b8829019800818975281/overview) is caused by this patch:
>
>   ../../../../llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/install/bin/../include/c++/v1/bit:142:5: error: static_assert failed due to requirement '__libcpp_is_unsigned_integer<i
>   nt>::value' "__countl_zero requires an unsigned integer type"
>       static_assert(__libcpp_is_unsigned_integer<_Tp>::value, "__countl_zero requires an unsigned integer type");
>       ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   ../../../../llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/install/bin/../include/c++/v1/__random/uniform_int_distribution.h:242:24: note: in instantiation of function template s
>   pecialization 'std::__countl_zero<int>' requested here
>       size_t __w = _Dt - __countl_zero(_Rp) - 1;
>                          ^
>   ../../../../llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/install/bin/../include/c++/v1/__random/uniform_int_distribution.h:206:17: note: in instantiation of function template s
>   pecialization 'std::uniform_int_distribution<bool>::operator()<std::linear_congruential_engine<unsigned int, 48271, 0, 2147483647>>' requested here
>           {return (*this)(__g, __p_);}
>                   ^
>   ../../src/tests/fidl/compatibility/compatibility_test.cc:592:43: note: in instantiation of function template specialization 'std::uniform_int_distribution<bool>::operator()<std::linea
>   r_congruential_engine<unsigned int, 48271, 0, 2147483647>>' requested here
>     s->primitive_types.b = bool_distribution(rand_engine);
>                                             ^
>
> Is there something wrong on our end that's causing this to happen, or is this from something incorrect with the patch?

I believe there's something wrong with the patch; specifically I think this'll fix it:

  diff --git a/libcxx/include/__random/uniform_int_distribution.h b/libcxx/include/__random/uniform_int_distribution.h
  index 55b4761637f0..169d2ed112d0 100644
  --- a/libcxx/include/__random/uniform_int_distribution.h
  +++ b/libcxx/include/__random/uniform_int_distribution.h
  @@ -230,8 +230,8 @@ typename uniform_int_distribution<_IntType>::result_type
   uniform_int_distribution<_IntType>::operator()(_URNG& __g, const param_type& __p)
   _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK
   {
  -    typedef typename conditional<sizeof(result_type) <= sizeof(uint32_t), uint32_t,
  -                                 typename make_unsigned<result_type>::type>::type _UIntType;
  +    typedef typename conditional<sizeof(result_type) <= sizeof(uint32_t), __identity<uint32_t>,
  +                                 make_unsigned<result_type> >::type::type _UIntType;
       const _UIntType _Rp = _UIntType(__p.b()) - _UIntType(__p.a()) + _UIntType(1);
       if (_Rp == 1)
           return __p.a();

I'm working on a regression test now. Shockingly, we have no tests for `uniform_int_distribution<T>` for //any// `T` except `int` and now `__{u,}int128_t` — not even `short`, let alone `bool`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114129