-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Proposal: String.Contains(char) #24068
Comments
Why is this needed? Is it purely for Ordinal and OrdinalIgnoreCase?
Does such an IndexOf overload exist that takes a char and a StringComparison? |
Yes
Oops, no. I added it to the proposal. But I don't this comparison is too important, it could be set aside. |
Just yesterday I had a use case for |
If the LINQ version is so much worse, is there room for a targeted optimization to improve/special case It? |
even beyond that, why not
IIRC, |
TSource would be char, not string. It could do "if (typeof(TSource) == typeof(char))" and then within that would need to do a cast/type check on the source instance to see if it was a string. That will add some overhead when TSource==char and the source isn't a string, so the question would be whether such overhead is measurable and worthwhile to optimize for the string case. |
I believe this is ready for review. |
FYI: The API review discussion was recorded - see https://youtu.be/o5JFZtgaJLs?t=1169 (1 min duration) |
Rationale and Usage
Finding a character in a string is a fairly common primitive operation. Unfortunately we lead users to the pit of failure because
mystring.Contains(char)
will bind to the Linq version, which can easily be 20x slower thanmystring.IndexOf(char) != -1
. By adding these to string, the next recompile will give a performance improvement.Proposed API
the implementations will simply be
Microbenchmark
Searching in 10 and 1000 character strings:
Variations
The StringComparison overload is in order to search case insensitively if desired. The IndexOf overload is needed to implement it. These could be discarded as the 90% case I would expect to not take a comparison.
The text was updated successfully, but these errors were encountered: