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

Enable Regex to use SearchValues<string> in compiled / source generator TryFindNextStartingPosition #88400

Closed
wants to merge 2 commits into from

Conversation

stephentoub
Copy link
Member

Depends on #88394
Fixes #85693

The 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.

@ghost
Copy link

ghost commented Jul 5, 2023

Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

Issue Details

Depends on #88394
Fixes #85693

The 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.

Author: stephentoub
Assignees: -
Labels:

area-System.Text.RegularExpressions

Milestone: -

// final length of the string, so keeping this small helps avoid problematic complexity.
const int MaxPrefixLength = 8;

// Limit based on what IndexOfAny is able to handle most efficiently. This can be updated in future if/when IndexOfAny improves further.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this point to the private constant in IndexOfAny that defines the value so that it aids in keeping the two values in sync?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which constant are you referring to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, so long since I reviewed this and now I'm trying to page back what I was thinking and can't recall. Should be fine to ignore, I believe I was just trying to suggest to add a pointer here to where that limit is set.

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor comments, but this LGTM.

Do we plan to add a benchmark for cases similar to the ones you are adding tests for? Also, if you have, can you share how this impacts the benchmarks we already have? I'm assuming with this we might be faster in some cases while having a slightly bigger memory footprint(for all arrays, strings, etc being allocated)?

…or TryFindNextStartingPosition

The 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.
@buyaa-n
Copy link
Contributor

buyaa-n commented Oct 18, 2023

@stephentoub is this PR blocked with something? Looks having a test failure.

@stephentoub
Copy link
Member Author

is this PR blocked with something?

There are some regressions to be investigated and fixed before it can be merged.

I will close it for now and revisit it soon.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 25, 2023
@stephentoub stephentoub deleted the regexteddy branch January 8, 2024 15:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use SearchValues<string> in Regex
4 participants