Skip to content
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

Use string.Contains(char) wherever possible - fallback to IndexOf if possible #49094

Closed
wants to merge 1 commit into from

Conversation

MeikTranel
Copy link

Fix remaining violations of the discussed string.Contains(string) -> string.Contains(char) rule.

Discussion

#47180

Additional context

#24068
dotnet/coreclr#15740
#36310

…the overload isnt available

string.Contains(char) offers better performance since by design it only needs a single iteration loop.
In preparation for consuming a new analyzer we want this to be fixed in the runtime beforehand.

Discussion: dotnet#47180
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@stephentoub
Copy link
Member

Thank you for the PR. These changes aren't worth the complication it adds (we don't want to churn the speech code base, and neither of these call sites are perf sensitive). I think we should not make these changes.

@MeikTranel
Copy link
Author

I can't say much about the performance sensitivity of System.Speech but i would generally agree. The NetframeworkUtils change would only surface if someone were to use EventLog.CreateEventSource in a tight loop which doesn't feel like something one would do.

@stephentoub
Copy link
Member

stephentoub commented Mar 4, 2021

Thanks. We can revisit when we import the new rule into the repo (thank you for working on it). We may want to enable it as a warning for the whole repo but suppress it for projects like these; we do that with a few other such rules.

@stephentoub stephentoub closed this Mar 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 3, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants