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

Use Spans on Go() methods for Code Gen engines #62324

Merged
merged 3 commits into from
Dec 4, 2021

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Dec 3, 2021

Contributes to #59629

Finishing the work to use Spans over the Emitter and Compiler now that @stephentoub work has been merged.

@ghost
Copy link

ghost commented Dec 3, 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

Contributes to #59629

Finishing the work to use Spans over the Emitter and Compiler now that @stephentoub work has been merged.

Author: joperezr
Assignees: -
Labels:

area-System.Text.RegularExpressions

Milestone: -

@joperezr
Copy link
Member Author

joperezr commented Dec 3, 2021

After this changes are in, for all 3 engines (interpreted, compiler, and emitter) we should only be using runtext twice per engine:

  • When initializing the local span on FindFirstChar()
  • When initializing the local span on Go(). For Code gen engines, this actually happens twice since we have the forks between simplifiedGo and CompleteGo, so technically for those engines we use runtext three times.

@joperezr joperezr self-assigned this Dec 3, 2021
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Looks good. There's going to be some conflicts with #62318, but whoever merges second can resolve those easily (mostly this is changing code that gets deleted there).

@joperezr
Copy link
Member Author

joperezr commented Dec 3, 2021

I'm fine being the one that resolves the conflicts so feel free to go ahead and merge, and I'll react to the changes.

@joperezr joperezr added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 3, 2021
@stephentoub
Copy link
Member

@joperezr, why don't you go ahead and get yours merged. Mine's blocked on a potential mono interpreter issue.

@joperezr joperezr force-pushed the CodeGenGoSpans branch 2 times, most recently from 5e00ef1 to 92f761c Compare December 3, 2021 22:16
@joperezr joperezr removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 3, 2021
@joperezr joperezr merged commit 9f4809e into dotnet:main Dec 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 3, 2022
@joperezr joperezr deleted the CodeGenGoSpans branch April 6, 2022 16:59
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.

3 participants