-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Add MemoryExtensions overloads with comparer #110197
base: main
Are you sure you want to change the base?
Conversation
Note regarding the
|
Note regarding the
|
6958f74
to
da01401
Compare
Copilot
AI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 17 changed files in this pull request and generated 3 comments.
Files not reviewed (9)
- src/libraries/System.Memory/tests/System.Memory.Tests.csproj: Language not supported
- src/libraries/System.Memory/tests/ReadOnlySpan/IndexOfSequence.T.cs: Evaluated as low risk
- src/libraries/System.Memory/tests/ReadOnlySpan/LastIndexOfSequence.T.cs: Evaluated as low risk
- src/libraries/System.Memory/tests/ReadOnlySpan/EndsWith.T.cs: Evaluated as low risk
- src/libraries/System.Memory/tests/ReadOnlySpan/SequenceCompareTo.T.cs: Evaluated as low risk
- src/libraries/System.Memory/tests/ReadOnlySpan/LastIndexOf.T.cs: Evaluated as low risk
- src/libraries/System.Memory/tests/ReadOnlySpan/IndexOf.T.cs: Evaluated as low risk
- src/libraries/System.Memory/tests/ReadOnlySpan/StartsWith.T.cs: Evaluated as low risk
- src/libraries/System.Memory/ref/System.Memory.cs: Evaluated as low risk
Comments suppressed due to low confidence (4)
src/libraries/System.Linq/src/System/Linq/Contains.cs:21
- The new overload for Contains using a comparer should have corresponding unit tests to ensure it behaves correctly with different comparers.
if (source.TryGetSpan(out ReadOnlySpan<TSource> span))
src/libraries/System.Linq/src/System/Linq/Contains.cs:32
- The check for value types should be removed as it is redundant. The logic for handling null comparers should be consistent regardless of whether TSource is a value type or not.
if (typeof(TSource).IsValueType)
src/libraries/System.Diagnostics.TraceSource/src/System/Diagnostics/TraceUtils.cs:22
- The Contains method used here does not specify a comparer, which might lead to incorrect behavior if the default equality comparer is not appropriate for the type of elements in supportedAttributes.
if (supportedAttributes is null || !supportedAttributes.Contains(key))
src/libraries/System.Memory/tests/ReadOnlySpan/Contains.T.cs:124
- The test should include a case where the comparer is null to ensure the method handles it correctly.
Assert.All(GetDefaultEqualityComparers<string>(), comparer => Assert.False(new ReadOnlySpan<string>(Array.Empty<string>()).Contains("a", comparer)));
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
Closes #28934