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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Couple of comments.

Also, I'm going to try this patch out on a significant code base just to confirm it doesn't result in crazy breakage -- requesting changes until that's done.

Ref for myself: 86111407&86111417



================
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:
> 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.


================
Comment at: libcxx/include/__random/is_valid_inttype.h:37
+
+#ifndef _LIBCPP_HAS_NO_INT128
+template<> struct __random_is_valid_inttype<__int128_t> : true_type {};
----------------
This is where we could document that we support this as an extension, in a comment.


================
Comment at: libcxx/test/std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/eval.pass.cpp:13
 
 // template<class _IntType = int>
 // class uniform_int_distribution
----------------
Any appetite for improving tests for other random distributions like this? It's *much* better than what we had before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114920