- Notifications
You must be signed in to change notification settings - Fork5.2k
Enable Regex to use SearchValues<string> in compiled / source generator for IgnoreCase multi-strings#98791
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
Enable Regex to use SearchValues<string> in compiled / source generator for IgnoreCase multi-strings#98791
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…or TryFindNextStartingPositionThe analyzer determines a set of prefixes that can start any match, and then uses SearchValues with IndexOfAny to find the next one from that set. It's currently only enabled for case-insensitive; we need to do some more perf validation before enabling for case-sensitive.
MihaZupan 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.
🎉
src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...ies/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexPrefixAnalyzer.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| // Arbitrary string length limit (with some wiggle room) to avoid creating strings that are longer than is useful and consuming too much memory. | ||
| constintMaxPrefixLength=8; |
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.
longer than is useful and consuming too much memory
This is mainly about not spending too many resources on the analysis part, not about the cost of the potentialSearchValues itself, right?
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.
Correct
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.
Well, "the longer than is useful" part was about SearchValues itself. Is that not the case?
...ies/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexPrefixAnalyzer.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...ies/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexPrefixAnalyzer.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.Text.RegularExpressions/tests/UnitTests/RegexPrefixAnalyzerTests.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
danmoseley commentedFeb 22, 2024
A lot of work to get to this point! I guess I should remeasure the rust benchmarks with all the alternations. |
ghost commentedFeb 22, 2024
Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions Issue DetailsThe analyzer determines a set of prefixes that can start any match, and then uses
Contributes to#85693
|
DrewScoggins commentedFeb 27, 2024 • 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.
Uh oh!
There was an error while loading.Please reload this page.
The analyzer determines a set of prefixes that can start any match, and then uses
SearchValues<string>with IndexOfAny to find the next one from that set. It's currently only enabled for case-insensitive; we need to do some more perf validation before enabling for case-sensitive.Contributes to#85693