[libcxx-commits] [PATCH] D114129: [libc++] Fix `uniform_int_distribution` for 128-bit result type

[Louis Dionne via Phabricator via libcxx-commits]

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

Thanks a lot for looking into this @fwolff.

The only issue I see with this patch is that the Standard says that instantiating any of the distributions with something that is not `short`, `int`, `long`, `long long`, `unsigned short`, `unsigned int`, `unsigned long`, or `unsigned long long` is undefined: http://eel.is/c++draft/rand#req.genl-1.5. Hence, this is technically an extension, and another valid approach would be to just `static_assert(_IntType is one of the aforementioned types);` from within the distributions. I'm curious to hear your thoughts on whether it's better to stick to the Standard or provide this as an extension (in which case we should probably annotate that it is an extension).

Requesting changes so we can have this discussion, however I'm fine with the patch if we decide to go for this approach.



================
Comment at: libcxx/include/__random/log2.h:61-66
+        typename conditional
+            <
+                sizeof(_UIntType) <= sizeof(unsigned long long),
+                    unsigned long long,
+                    __uint128_t
+            >::type,
----------------
Or something like that -- the current formatting looks really weird with the `<` alone on its own line.


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

https://reviews.llvm.org/D114129