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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D125283#3532818 <https://reviews.llvm.org/D125283#3532818>, @cjdb wrote:

> I'm not sure why there's so much resistance to reverting. libc++:
>
> 1. has users with broken code right now;
> 2. actively supports `uniform_int_distribution<__int128_t>`, **which is also undefined** (I don't get why you consider `signed char`, `unsigned char`, `char`, and `bool` to be problematic, but not `__int128_t` and `unsigned __int128_t`);
> 3. has licence to make it an extension without Microsoft/STL's support;
>
> You can also propose standardising this right now.

I don't know whether our distributions actually work with these types. This explicit rejection was added in D114920 <https://reviews.llvm.org/D114920>, which was itself prompted by discussions on D114129 <https://reviews.llvm.org/D114129>, whose goal was to fix http://llvm.org/PR51520 (an overflow in `uniform_int_distribution<__int128_t>`). What tells us that we don't have similar problems with `uniform_int_distribution<char>` & friends. They are not tested, so anything is possible. Then, add the fact that the Standard made those undefined, which I consider a red flag; without diving into the implementation of those distributions, I'm thinking there might be a good reason why they didn't require implementations to support those types, e.g. maybe the usual algorithms don't work properly for types of that size.

So, given this uncertainty, I think it's better for everyone (our users especially) to get this compile-time error. However, if this patch showed that our implementation indeed supports these types properly, it would become a simple question of "do we want to support them as an extension, like we do for `__int128_t`", and that's easier to answer.

If you can add tests to show that distributions work with these types, I'd be OK with that. We still have until June 8th to cherry-pick to LLVM 14, so we could even backport to LLVM 14 and avoid breaking users for just one release (which would be very annoying indeed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125283