Skip to content

Fix double-negatives with MatchCharacterClass #62636

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

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

stephentoub
Copy link
Member

Fixes #62622
cc: @joperezr

@ghost
Copy link

ghost commented Dec 10, 2021

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

Issue Details

Fixes #62622
cc: @joperezr

Author: stephentoub
Assignees: -
Labels:

area-System.Text.RegularExpressions

Milestone: -

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.

LGTM, generated code does look much better.

Separate comment: even though I know the purpose of this is purely for readability purposes (therefore not very applicable to Compiled) would it make sense to make the changes in MatchCharacterClass on Compiled engine as well just for consistency purposes?

@joperezr
Copy link
Member

BTW it's been a bit since I last saw that many XORs in C# 😄

@stephentoub
Copy link
Member Author

BTW it's been a bit since I last saw that many XORs in C#

If you need it, flaunt it 😄

Separate comment: even though I know the purpose of this is purely for readability purposes (therefore not very applicable to Compiled) would it make sense to make the changes in MatchCharacterClass on Compiled engine as well just for consistency purposes?

Sure, I can make that change.

Also add missing set description rendering for \d, \D
@stephentoub
Copy link
Member Author

Separate comment: even though I know the purpose of this is purely for readability purposes (therefore not very applicable to Compiled) would it make sense to make the changes in MatchCharacterClass on Compiled engine as well just for consistency purposes?

Sure, I can make that change.

Actually, I took a look at it, and best case it saves us a single ceq operation, while adding additional complexity. This particular function is one with a signature that already differs from what's in RegexCompiled, e.g. to be able to pass out additional declarations, so I don't think this really hampers the consistency much, and I'd prefer not to augment RegexCompiler with it.

@stephentoub stephentoub merged commit 4c9b71b into dotnet:main Dec 10, 2021
@stephentoub stephentoub deleted the fixinversion branch December 10, 2021 22:14
@ghost ghost locked as resolved and limited conversation to collaborators Jan 10, 2022
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.

Regex source generator could produce better negated conditions in some cases
2 participants