Add generic Random NextInteger/NextBinaryFloat APIs#127462
Add generic Random NextInteger/NextBinaryFloat APIs#127462stephentoub wants to merge 1 commit intomainfrom
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds new generic numeric APIs to System.Random to generate random integer values for any IBinaryInteger<T> and random IEEE-754 binary floating-point values for any IBinaryFloatingPointIeee754<T>.
Changes:
- Add
Random.NextInteger<T>(),Random.NextInteger<T>(T maxValue), andRandom.NextInteger<T>(T minValue, T maxValue)with fast paths for common integer types and generic rejection-sampling fallbacks. - Add
Random.NextBinaryFloat<T>()with fast paths forfloat/double/NFloatand a generic implementation that guarantees results in[0, 1). - Update reference assembly API surface and add unit tests covering edge cases, large ranges, and derived
Randomdispatch behavior.
Show a summary per file
| File | Description |
|---|---|
src/libraries/System.Private.CoreLib/src/System/Random.cs |
Implements the new generic integer and binary-float generation APIs, including fast paths and fallbacks. |
src/libraries/System.Runtime/ref/System.Runtime.cs |
Exposes the new Random APIs in the System.Runtime reference assembly. |
src/libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/Random.cs |
Adds coverage for argument validation, boundary/range behavior, and derived Random virtual dispatch; also introduces helper numeric wrapper types for custom reference-type coverage. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 3
| return T.Zero; | ||
| } | ||
|
|
||
| Debug.Assert(T.IsPositive(T.MaxValue)); |
There was a problem hiding this comment.
This feels like it needs to throw if we are preventing some NeverPositiveInteger from working.
| /// Unlike <see cref="Next()"/>, which returns an <see cref="int"/> that is less than <see cref="int.MaxValue"/>, | ||
| /// <c>NextInteger<int>()</c> returns an <see cref="int"/> in the inclusive range from zero through | ||
| /// <see cref="int.MaxValue"/> and may return <see cref="int.MaxValue"/>. | ||
| /// <typeparamref name="T"/> must use a two's complement representation for signed values. |
There was a problem hiding this comment.
This comment seems incorrect and/or misleading.
The underlying type can use whatever storage format it wants, nothing prevents some bool signed; uint magnitude; value from functioning.
Rather, the consideration is that it must behave as if it were a two's complement value with all values in range being representable (see also BigInteger), which is a general expectation of IBinaryInteger<TSelf> due to APIs like lzcnt, popcnt, tzcnt, shift, etc
This is because we functionally determine the number of bits needed for [0, MaxValue] and then use T.ReadLittleEndian to construct the value and that won't work correctly for something that has non-linear distribution or which violates other IBinaryInteger expectations
| int bitLength = T.MaxValue.GetShortestBitLength(); | ||
| int byteCount = (bitLength + 7) >> 3; |
There was a problem hiding this comment.
nit: The bit arithmetic here is efficient, but it's also harder to read
Can we either add the appropriate comments or cast to uint and do the normal operation (i.e. (bitLength + 7) / 8) so the JIT optimizations will kick in?
| if (value <= T.MaxValue) | ||
| { | ||
| return value; | ||
| } |
There was a problem hiding this comment.
This seems superfluous, a type is "invalid" and will exhibit undefined behavior if MaxValue isn't actually MaxValue
tannergooding
left a comment
There was a problem hiding this comment.
Some minor nits, but overall looks good.
I think we should remove the Debug.Assert from the public API and validate it properly however, since we'll never trigger that assert ourselves but a user might.
Adds generic numeric APIs to
System.Random:NextInteger<T>()NextInteger<T>(T maxValue)NextInteger<T>(T minValue, T maxValue)NextBinaryFloat<T>().Fixes #75431