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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a subscriber: ldionne.
Quuxplusone added a comment.

LGTM mod nits! Please wait for @Mordante or @ldionne to approve.
For the record, do we think that there is an implied "TODO" here to look into the situation for all other integer distributions, such as `binomial_distribution`?
And if so, are you (@fwolff) volunteering to tackle that?



================
Comment at: libcxx/include/__bits:53
+      : __libcpp_clz((unsigned long long)(__x >> 64));
+}
+
----------------
Nit: I personally prefer to see `(X)-1` rather than `(X)~0`, since I always mentally read the latter as `(X)~0u` (which would be wrong) and then correct myself. :) Alternatively, in this case I guess you could just use `~0uLL` and save yourself some parentheses.


================
Comment at: libcxx/test/std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/int128.pass.cpp:81
+    }
+  }
+
----------------
Tiny nits throughout:
- `a <= n && n <= b` is easier on the eyes than `n >= a && n <= b`. https://quuxplusone.github.io/blog/2021/05/05/its-clamping-time/
- I'd prefer not to see the extraneous `const` on lines 42, 43, 49, 58, 59, 65.
- I'd prefer `T(a, b)` over `T{a, b}` on lines 61, 75, and arguably 60, 74 (since we're not worried about most-vexing-parse, and our `T`s here are not sequences, arrays, or aggregates).
- Perhaps rename `all_in_range` to `all_in_64bit_range` or `all_in_narrower_range` or something like that, to indicate more clearly why we //don't// expect all the output to be "in range"?


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

https://reviews.llvm.org/D114129