Address PR feedback on Random.NextXx#127849
Open
stephentoub wants to merge 1 commit intomainfrom
Open
Conversation
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates System.Random’s new generic integer APIs (NextInteger<T>) to address post-merge feedback, primarily around documenting/enforcing assumptions for supported IBinaryInteger<T> implementations and simplifying the full-range generation path.
Changes:
- Updates
NextInteger<T>type requirements in XML docs (two’s-complement/full-range wording). - Adjusts
NextInteger<T>()to validateT.MaxValuesign and simplifies generation (removes rejection loop). - Refactors range generation logic (ternary simplification) and folds the generic rejection-sampling helper into
NextBinaryIntegerInRange.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Random.cs | Tweaks docs/validation and refactors generic integer random generation logic for NextInteger<T> and helpers. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 3
| /// <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. | ||
| /// <remarks><typeparamref name="T"/> must behave as if it were a two's complement value with all values in range being representable.</remarks> |
| } | ||
|
|
||
| Debug.Assert(T.IsPositive(T.MaxValue)); | ||
| throw new ArgumentOutOfRangeException(nameof(T), SR.Format(SR.ArgumentOutOfRange_Generic_MustBeNonNegative, nameof(T.MaxValue), T.MaxValue)); |
Comment on lines
+223
to
+226
| 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
+199
to
+202
| if (T.MaxValue == T.Zero) | ||
| { | ||
| return T.Zero; | ||
| } |
Member
There was a problem hiding this comment.
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);
tannergooding
approved these changes
May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Somehow the extra commit didn't get into the PR before it merged.
cc: @tannergooding