Fix NIndex/NRange ToString truncation on 64-bit: cast to nuint instead of uint#124294
Fix NIndex/NRange ToString truncation on 64-bit: cast to nuint instead of uint#124294tannergooding merged 2 commits intomainfrom
Conversation
…int, add Theory tests Co-authored-by: vcsjones <361677+vcsjones@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-numerics-tensors |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect string formatting for NIndex/NRange on 64-bit platforms where values larger than uint.MaxValue were being truncated due to casts to uint during formatting.
Changes:
- Update
NIndex.ToStringFromEnd()to format using(nuint)Valuerather than(uint)Value. - Update
NRange.ToString()to format start/end using(nuint)Start.Value/(nuint)End.Valuerather thanuint. - Expand/adjust unit tests to cover 64-bit values above
uint.MaxValue, converting prior[Fact]coverage into[Theory]+[InlineData]and adding 64-bit conditional cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Numerics.Tensors/src/System/Buffers/NIndex.cs | Fixes 64-bit truncation in ToStringFromEnd() by formatting via nuint. |
| src/libraries/System.Numerics.Tensors/src/System/Buffers/NRange.cs | Fixes 64-bit truncation in ToString() by formatting start/end via nuint. |
| src/libraries/System.Numerics.Tensors/tests/NIndexTests.cs | Adds theory-based coverage and conditional 64-bit test cases for large values. |
| src/libraries/System.Numerics.Tensors/tests/NRangeTests.cs | Adds theory-based coverage and conditional 64-bit test cases for large range endpoints. |
|
@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR. |
🤖 Copilot Code ReviewHolistic AssessmentMotivation: The bug is real and clearly reproducible — Approach: Changing Summary: ✅ LGTM. The three cast fixes are correct, minimal, and well-tested. All findings below are positive confirmations or out-of-scope observations. Detailed Findings✅ Correctness — Cast fix is correct in all three locations
The ✅ Test quality — Good coverage with appropriate conditional attributesThe tests cover both 💡 Out-of-scope observation —
|
@copilot can you open a separate PR to address this. |
I'm not able to open new PRs. Could you open an issue for tracking the |
|
I will open a separate PR for the offset / length issue. |
|
This one should be backported, yes? |
|
@tannergooding Up to you, but there are a couple of things you may want to back port when things are merged.
|
|
I'm fine with taking them all together, but I do think we should take them for bar check at least. -- CC. @jeffhandley |
Description
NIndex.ToStringFromEnd()andNRange.ToString()castnintvalues touintbefore formatting, truncating values exceedinguint.MaxValueon 64-bit platforms:Fix
(uint)Value→(nuint)ValueinToStringFromEnd()(uint)Start.Value→(nuint)Start.Valueand(uint)End.Value→(nuint)End.ValueinToString()Tests
[Fact] ToStringTestwith[Theory]+[InlineData]for bothNIndexTestsandNRangeTests[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))]tests exercising values aboveuint.MaxValue💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.