This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Merged
Conversation
| * a RegexNode for the type of atom scanned. | ||
| */ | ||
| internal RegexNode ScanBackslash() | ||
| internal RegexNode ScanBackslash(bool scanOnly = false) |
There was a problem hiding this comment.
IMO, optional parameters for these internal APIs aren't a great idea. By being non-optional it ensures there isn't a caller missed.
| } | ||
|
|
||
| // Try to parse backreference: \<1> or \<cap> | ||
| // Try to parse backreference: \<1> |
There was a problem hiding this comment.
the comment seemed right. it was just referring to the if and the else section.
CyrusNajmabadi
approved these changes
Feb 4, 2018
Member
Author
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
* Fix with test * Avoid excess allocations * Negative test * Exclude from NETFX * Avoid default params * NETFX Commit migrated from dotnet/corefx@80e220f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix https://github.com/dotnet/corefx/issues/26501
In
(cat)(\c[*)(dog)the\c[should match the[control character ie\u001b. However it was rejecting the pattern because it does a pre-scan for capturing groups. In this phase it assumes that\always escaped a single character, so it treats the[as the start of a character class, which is ultimately never closed.The fix is to call into the real scanner for escape characters. It is necessary to pass a flag indicating we are scanning. This is to avoid validating that backreferences refer to valid capturing groups (these are not all set up until the pre-scanning is complete) and also to avoid allocating objects that aren't needed in this phase. We already do something very similar with ScanCharClass.
Ideally there would not be a pre-scan but I imagine that would be a substantial change.
This causes more code to run during the pre-scan when a backslash is encountered. To verify this does not impact perf I used the perf test in #26827
Before
After this and #26543
Elapsed difference is within noise level and allocations are negligibly increased.