[libcxx-commits] [PATCH] D114920: [libc++] Explicitly reject `uniform_int_distribution<bool>`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D114920#3177676 <https://reviews.llvm.org/D114920#3177676>, @Quuxplusone wrote:

> @ldionne: Makes perfect sense to me. Would you care to suggest wording for a release note? Perhaps
>
>   (API Changes)
>   The integer distributions ``binomial_distribution``, ``discrete_distribution``,
>   ``geometric_distribution``, ``negative_binomial_distribution``, ``poisson_distribution``,
>   and ``uniform_int_distribution`` now conform to the Standard by rejecting
>   template parameter types other than ``short``, ``int``, ``long``, ``long long``,
>   (as an extension) ``__int128_t``, and the unsigned versions thereof.
>   In particular, ``uniform_int_distribution<int8_t>`` is no longer supported.

Yes, that sounds perfect to me.

> Also, since "everything but the static_asserts" is really just "the test file," shall I go ahead and land the test-file change by itself, soonish?

Yes! You can do this as a NFC commit and repurpose this patch to be only the static asserts and the release note, and fixing the benchmarks.



================
Comment at: libcxx/include/__random/is_valid_inttype.h:21-40
+// [rand.req.genl]/1.5:
+// The effect of instantiating a template that has a template type parameter
+// named IntType is undefined unless the corresponding template argument is
+// cv-unqualified and is one of short, int, long, long long, unsigned short,
+// unsigned int, unsigned long, or unsigned long long.
+
+template<class> struct __random_is_valid_inttype : false_type {};
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > I'm not thrilled about this helper or its placement/naming, but it seemed like the best option.
> > > Incidentally, this also gives people a sneaky libc++ internal detail that they can (unsupportedly) specialize in order to re-enable the removed functionality for specific user-defined types.
> > > Should this helper be named `__libcpp_*`?
> > Yeah, let's use `__libcpp_*`. I don't like that name, but I think it's worth doing it if it reduces the likelihood of users messing around with our internal details.
> Will rename to `__libcpp_random_is_valid_inttype`.
> 
> Also yuck, `libcxx/benchmarks/vector_operations.bench.cpp` uses `uniform_int_distribution<char>` in order to get random example data for its vectors. Maybe we should just support that. It would match libstdc++, too.
> 
> `uniform_int_distribution<char/int8_t/uint8_t>`: GCC accepts, MSVC rejects with static_assert.
> `uniform_int_distribution<bool>`: GCC and MSVC both reject, with ugly template error message.
So basically we'd only exclude `bool`? IMO we should strive for strict conformance, however we should also write a half-page paper to support it for `char` and other small types cause it kind of makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114920