Skip to content

Address PR feedback on Random.NextXx #127849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stephentoub wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −31
Open

Address PR feedback on Random.NextXx #127849

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 18 additions & 31 deletions src/libraries/System.Private.CoreLib/src/System/Random.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,16 +190,19 @@ public virtual void NextBytes(byte[] buffer)
/// Unlike <see cref="Next()"/>, which returns an <see cref="int"/> that is less than <see cref="int.MaxValue"/>,
/// <c>NextInteger&lt;int&gt;()</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.
/// <remarks><typeparamref name="T"/> must behave as if it were a two's complement value with all values in range being representable.</remarks>
/// </remarks>
public T NextInteger<T>() where T : IBinaryInteger<T>, IMinMaxValue<T>
{
if (T.MaxValue == T.Zero)
if (!T.IsPositive(T.MaxValue))
{
return T.Zero;
}
if (T.MaxValue == T.Zero)
{
return T.Zero;
}
Comment on lines +199 to +202
Copy link
Copy Markdown
Member

@tannergooding tannergooding May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check should be unnecessary as IsPositive strictly covers Zero. The only thing it'd catch is an edge case where a user has a one's complement integer that can only represent negative values and so MaxValue is -0, but I'm not sure that's worth handling since we say this returns a positive number (and -0 is not positive)

/// <summary>Determines if a value represents zero or a positive real number.</summary>
/// <param name="value">The value to be checked.</param>
/// <returns><c>true</c> if <paramref name="value" /> represents (positive) zero or a positive real number; otherwise, <c>false</c>.</returns>
/// <remarks>
///     <para>If this type has signed zero, then <c>-0</c> is not considered positive, but <c>+0</c> is.</para>
///     <para>This function returning <c>false</c> does not imply that <see cref="IsNegative(TSelf)" /> will return <c>true</c>. A complex number, <c>a + bi</c> for non-zero <c>b</c>, is not positive nor negative</para>
/// </remarks>
static abstract bool IsPositive(TSelf value);


Debug.Assert(T.IsPositive(T.MaxValue));
throw new ArgumentOutOfRangeException(nameof(T), SR.Format(SR.ArgumentOutOfRange_Generic_MustBeNonNegative, nameof(T.MaxValue), T.MaxValue));
}

int bitLength = T.MaxValue.GetShortestBitLength();
int byteCount = (bitLength + 7) >> 3;
Expand All @@ -217,17 +220,10 @@ public T NextInteger<T>() where T : IBinaryInteger<T>, IMinMaxValue<T>

try
{
while (true)
{
NextBytes(bytes);
bytes[^1] &= topMask;
NextBytes(bytes);
bytes[^1] &= topMask;

T value = T.ReadLittleEndian(bytes, isUnsigned: true);
if (value <= T.MaxValue)
{
return value;
}
}
return T.ReadLittleEndian(bytes, isUnsigned: true);
Comment on lines +223 to +226
}
finally
{
Expand All @@ -248,7 +244,7 @@ public T NextInteger<T>() where T : IBinaryInteger<T>, IMinMaxValue<T>
/// [0, <paramref name="maxValue"/>). However, if <paramref name="maxValue"/> equals zero, zero is returned.
/// </returns>
/// <exception cref="ArgumentOutOfRangeException"><paramref name="maxValue"/> is less than zero.</exception>
/// <remarks><typeparamref name="T"/> must use a two's complement representation for signed values.</remarks>
/// <remarks><typeparamref name="T"/> must behave as if it were a two's complement value with all values in range being representable.</remarks>
public T NextInteger<T>(T maxValue) where T : IBinaryInteger<T>
{
ArgumentOutOfRangeException.ThrowIfNegative(maxValue);
Expand All @@ -268,24 +264,20 @@ public T NextInteger<T>(T maxValue) where T : IBinaryInteger<T>
/// equals <paramref name="maxValue"/>, <paramref name="minValue"/> is returned.
/// </returns>
/// <exception cref="ArgumentOutOfRangeException"><paramref name="minValue"/> is greater than <paramref name="maxValue"/>.</exception>
/// <remarks><typeparamref name="T"/> must use a two's complement representation for signed values.</remarks>
/// <remarks><typeparamref name="T"/> must behave as if it were a two's complement value with all values in range being representable.</remarks>
public T NextInteger<T>(T minValue, T maxValue) where T : IBinaryInteger<T>
{
if (minValue > maxValue)
{
ThrowMinMaxValueSwapped();
}

T range = maxValue - minValue;

// For signed types, subtraction may overflow when the range exceeds T.MaxValue.
// T.IsNegative(range) detects this. Fall back to full-width generation.
if (T.IsNegative(range))
{
return NextBinaryIntegerFullRange(minValue, maxValue);
}

return NextBinaryIntegerInRange(range) + minValue;
T range = maxValue - minValue;
return T.IsNegative(range) ?
NextBinaryIntegerFullRange(minValue, maxValue) :
NextBinaryIntegerInRange(range) + minValue;
}

/// <summary>Generates a random value in [T.Zero, maxExclusive) where maxExclusive is non-negative.</summary>
Expand Down Expand Up @@ -331,12 +323,7 @@ private T NextBinaryIntegerInRange<T>(T maxExclusive) where T : IBinaryInteger<T
}

// Generic fallback for large ulong, nuint, Int128, UInt128, BigInteger, etc.
return NextBinaryIntegerRejectionSampling(maxExclusive);
}

/// <summary>Generic rejection sampling for arbitrary <see cref="IBinaryInteger{T}"/> types.</summary>
private T NextBinaryIntegerRejectionSampling<T>(T maxExclusive) where T : IBinaryInteger<T>
{
if (maxExclusive == T.Zero)
{
return T.Zero;
Expand All @@ -345,7 +332,7 @@ private T NextBinaryIntegerRejectionSampling<T>(T maxExclusive) where T : IBinar
Debug.Assert(T.IsPositive(maxExclusive));

int bitLength = maxExclusive.GetShortestBitLength();
int byteCount = (bitLength + 7) >> 3;
int byteCount = (int)(((uint)bitLength + 7) / 8);

// Compute mask for the top byte to reduce rejection rate.
int topBits = bitLength & 7;
Expand Down
Loading