- Notifications
You must be signed in to change notification settings - Fork481
Add analyzer/fixer to suggest using cached SearchValues instances#6898
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedAug 29, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #6898 +/- ##==========================================- Coverage 96.40% 96.40% -0.01%========================================== Files 1403 1402 -1 Lines 331075 331964 +889 Branches 10893 11036 +143 ==========================================+ Hits 319166 320015 +849- Misses 9178 9186 +8- Partials 2731 2763 +32 |
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseSearchValues.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
buyaa-n 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.
Thank you for the great test coverage, in general LGTM, I would let@mavasani take a look to the new utilities and some of approaches used in the analyzer/fixer.
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/UseSearchValuesTests.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/UseSearchValuesTests.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Performance/CSharpUseSearchValues.Fixer.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
buyaa-n commentedSep 6, 2023
FYI@mavasani we want to add this new analyzer into .NET 8 as |
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseSearchValues.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
mavasani commentedSep 7, 2023
Overall, this looks good to me. I primarily looked at the analyzer and test code, just skimmed over the code fixer code. |
MihaZupan commentedSep 8, 2023
/backport to release/8.0.1xx |
Started backporting to release/8.0.1xx:https://github.com/dotnet/roslyn-analyzers/actions/runs/6124792900 |
Uh oh!
There was an error while loading.Please reload this page.
Fixesdotnet/runtime#78587
Flags 3 cases in dotnet/runtime, all of them in projects where we've avoided making the switch because they multi-target to older TFMs (
SearchValuesis a .NET 8+ API).The analyzer flags cases like
for all
{Last}IndexOfAny{Except}andContainsAny{Except}overloads forbyte/charand suggests that they be rewritten asWe'll also rewrite the
string.IndexOfAny(char[])overload to usestring.AsSpan().IndexOfAny(SearchValues)instead.We won't do the same for
IndexOfAny(char[], int)orIndexOfAny(char[], int, int)as they would require us to inject more logic to account for the differences in behavior (returning offset from start vs. from 0)If the values are specified by a constant in an appropriate scope, we'll reuse said constant to feed the
SearchValues.Create.If we're not using the original member and there are now no other uses, we'll remove it.
For a full list of patterns that we do/don't recognize, seethis test source.
Potential improvements:
IDE0300will suggest replacingnew[] { 'a', 'b', 'c' }with['a', 'b', 'c'], whereas this analyzer does not recognize collection literals.