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.

Sign up for GitHub

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

Jump to bottom

Add Span.TryWrite, StringBuilder.Append, and String.Create interpolated strings support #51653

Merged
merged 4 commits into from
Jul 13, 2021
Merged

Add Span.TryWrite, StringBuilder.Append, and String.Create interpolated strings support #51653

merged 4 commits into from
Jul 13, 2021

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Apr 21, 2021 •
edited
Loading

Fixes #50635
Closes #50674
Fixes #51962
Contributes to #53211

cc: @GrabYourPitchforks, @bartonjs, @333fred, @jaredpar

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@stephentoub stephentoub added this to the 6.0.0 milestone Apr 21, 2021
@stephentoub stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 22, 2021
@stephentoub
Copy link
Member Author

Given the user-facing public surface area in the PR, I think it makes sense to hold off on merging this until the accompanying compiler functionality is ready to go. @333fred, do you have a rough ETA for when you expect the compiler support to be merged?

@333fred
Copy link
Member

333fred commented Apr 23, 2021

Given the user-facing public surface area in the PR, I think it makes sense to hold off on merging this until the accompanying compiler functionality is ready to go. @333fred, do you have a rough ETA for when you expect the compiler support to be merged?

Probably after you turn into a pumpkin.

@stephentoub
Copy link
Member Author

Just wondering if we're talking days, weeks, or months...

@333fred
Copy link
Member

333fred commented Apr 23, 2021

Just wondering if we're talking days, weeks, or months...

I would think weeks.

@stephentoub stephentoub force-pushed the interpolatedstringspart2 branch 2 times, most recently from 019673d to c3eb0b1 Compare May 9, 2021 21:14
@stephentoub stephentoub removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 10, 2021
@stephentoub stephentoub force-pushed the interpolatedstringspart2 branch 2 times, most recently from 32a8ebf to c5fc06a Compare May 10, 2021 20:07
@stephentoub stephentoub mentioned this pull request May 24, 2021
Merged
@runfoapp runfoapp bot mentioned this pull request May 28, 2021
Closed
@stephentoub
Copy link
Member Author

Just waiting to merge this until the relevant support in the compiler is merged...

@333fred
Copy link
Member

333fred commented Jun 11, 2021

Just waiting to merge this until the relevant support in the compiler is merged...

Soon ™️. I spent today doing initial binding for arguments, Monday afternoon after LDM I should hopefully finish lowering and get a PR out for that. Then it's just some fit and finish (nullable, definite assignment, IOperation) and it should be good to merge.

@stephentoub
Copy link
Member Author

Thanks for reviewing again, @bartonjs.

@stephentoub stephentoub force-pushed the interpolatedstringspart2 branch 3 times, most recently from a71cd5c to 6ada340 Compare July 1, 2021 13:28
@stephentoub stephentoub mentioned this pull request Jul 9, 2021
Closed
333fred
333fred reviewed Jul 9, 2021
View reviewed changes
Directory.Build.props Outdated Show resolved Hide resolved
@stephentoub stephentoub force-pushed the interpolatedstringspart2 branch 5 times, most recently from aba49af to 6115289 Compare July 12, 2021 04:04
@stephentoub stephentoub mentioned this pull request Jul 12, 2021
Merged
@runfoapp runfoapp bot mentioned this pull request Jul 13, 2021
Closed
@stephentoub stephentoub merged commit dfd618d into dotnet:main Jul 13, 2021
@stephentoub stephentoub deleted the interpolatedstringspart2 branch July 13, 2021 14:12
@runfoapp runfoapp bot mentioned this pull request Jul 15, 2021
Closed
@kunalspathak
Copy link
Member

@stephentoub - Could you confirm if the regressions on arm64 are from this change?

DrewScoggins/performance-2#7638

@stephentoub
Copy link
Member Author

DrewScoggins/performance-2#7638 (comment)

@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@am11 am11 am11 left review comments

@333fred 333fred 333fred left review comments

@bartonjs bartonjs bartonjs approved these changes

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks approved these changes

Assignees
No one assigned
Labels
new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
6 participants
@stephentoub @333fred @kunalspathak @GrabYourPitchforks @am11 @bartonjs