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

Is `uniform_int_distribution` really the only distribution with this kind of problem? What about, I dunno, `binomial_distribution`?



================
Comment at: libcxx/include/__bits:55
+
+#endif
+
----------------



================
Comment at: libcxx/include/__random/uniform_int_distribution.h:296
+#endif
+                                 >::type _UIntType;
     const _UIntType _Rp = _UIntType(__p.b()) - _UIntType(__p.a()) + _UIntType(1);
----------------
Would this work if it were simply
```
typedef typename conditional<sizeof(result_type) <= 4, uint32_t,
    typename make_unsigned<result_type>::type>::type _UIntType;
```
? That is, I think the old code was just assuming that for types larger than 32 bits, `typename make_unsigned<result_type>::type` was by definition `uint64_t`; now that that's false, we can just make the substitution.


================
Comment at: libcxx/test/std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/int128.pass.cpp:28
+    auto e = std::default_random_engine{};
+    auto d = std::uniform_int_distribution<__int128_t>{};
+    bool all_in_range = true;
----------------
- I think it's worth verifying that `d.min() == 0 && d.max() == INT128_MAX` (however we spell that). Similarly on line 40.

- Shall we add a case where the constructor arguments to `uniform_int_distribution<__[u]int128_t>` are outside the range of int64/uint64?

- Style nit: Please use `int` not `auto` on lines 30 and 41, and `__int128_t` not `auto` on lines 31 and 42.

- Shall we check `uniform_int_distribution<__uint128_t>` while we're at it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114129