Fix ISXB-1792: HaveDuplicateReferences inner loop bound (n < count no…#2376
Fix ISXB-1792: HaveDuplicateReferences inner loop bound (n < count no…#2376
Conversation
…t n < count - i) - Inner loop was terminating too early, missing duplicate checks for later elements. - Add Utilities_HaveDuplicateReferences_DetectsDuplicatesInFullRange test including regression for dup at end. Made-with: Cursor
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
| for (var i = 0; i < count; ++i) | ||
| { | ||
| var element = first[i]; | ||
| for (var n = i + 1; n < count - i; ++n) | ||
| for (var n = i + 1; n < count; ++n) |
There was a problem hiding this comment.
While this PR successfully fixes the inner loop's end condition, the method still entirely ignores the index parameter. Both the outer and inner loops iterate up to count but start at 0 instead of index, meaning they will scan the wrong range of elements if index > 0.
Should this be updated to respect the index parameter?
for (var i = index; i < index + count; ++i)
{
var element = first[i];
for (var n = i + 1; n < index + count; ++n)
{
if (ReferenceEquals(element, first[n]))
return true;
}
}🤖 Helpful? 👍/👎
| withDup[2] = withDup[0]; // duplicate at 0 and 2 | ||
| Assert.That(withDup.HaveDuplicateReferences(0, 3), Is.True); |
There was a problem hiding this comment.
Have you considered adding a test case with a non-zero starting index (e.g., index: 1)? This would ensure the bounds are correctly respected. Because the implementation of HaveDuplicateReferences currently ignores the index parameter, a test relying on an offset would fail, helping to expose the missing offset bug.
🤖 Helpful? 👍/👎
| for (var n = i + 1; n < count - i; ++n) | ||
| for (var n = i + 1; n < count; ++n) | ||
| { | ||
| if (ReferenceEquals(element, first[n])) |
There was a problem hiding this comment.
Because ReferenceEquals is used for comparison, this method will always return false for value types (like int or struct) because they are boxed into unique objects when passed as object arguments. Have you considered adding a where TFirst : class constraint to the generic type to prevent accidental misuse with value types?
🤖 Helpful? 👍/👎
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2376 +/- ##
========================================
Coverage 77.90% 77.91%
========================================
Files 476 481 +5
Lines 97613 97671 +58
========================================
+ Hits 76048 76102 +54
- Misses 21565 21569 +4 Flags with carried forward coverage won't be shown. Click here to find out more.
|
1 participant
Link: https://jira.unity3d.com/browse/ISXB-1792
Why it’s easy: The ticket describes the exact bug and fix. The inner loop uses n < count - i but should use n < count, so the loop doesn’t cover the full range and duplicate detection is wrong.
Fix: In ArrayHelpers.cs (around line 118), change the inner loop condition from n < count - i to n < count.
Extra: Reporter suggests adding a test. Single file, one-line logic fix.
Made-with: Cursor
Description
Please fill this section with a description what the pull request is trying to address and what changes were made.
Testing status & QA
Please describe the testing already done by you and what testing you request/recommend QA to execute. If you used or created any testing project please link them here too for QA.
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.