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

Add S.R.CompilerServices.InterpolatedStringBuilder #51086

Merged
merged 5 commits into from
Apr 15, 2021

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Apr 11, 2021

Closes #50601
Closes #28945
Closes #30547
Closes #26374
Contributes to #14484 (second half of this will close it)

Draft PR as the API should be reviewed on Tuesday and so may be tweaked as a result of that.

This also includes a few Create overloads in support of #50635, but that made sense to include in the implementation and testing here.

cc: @333fred, @GrabYourPitchforks, @tannergooding, @pgovind

@stephentoub stephentoub added this to the 6.0.0 milestone Apr 11, 2021
@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.

@GrabYourPitchforks
Copy link
Member

Well of course the +1294 line change will be the one that passes CI flawlessly. :)

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

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

This essentially makes a ValueStringBuilder-equivalent type public, which means we'll probably want to lock down #25587 and #50389 ASAP. It'd be weird for us to drag our feet on that if we're trying to get this in soon.

@GrabYourPitchforks
Copy link
Member

I didn't see any problems in the implementation, BTW. Also skimmed the unit tests and they looked fine. My feedback was mainly a bunch of questions.

@stephentoub
Copy link
Member Author

This essentially makes a ValueStringBuilder-equivalent type public, which means we'll probably want to lock down #25587 and #50389 ASAP. It'd be weird for us to drag our feet on that if we're trying to get this in soon.

I don't think they need to be tied together. We have a lot of things in System.Runtime.InteropServices and System.Runtime.CompilerServices that are effectively unsafe; this is just one more. That said, if we ever did want to mark this type as non-copyable, that would have a significant impact on the shape of the APIs in part 2, and what the compiler needs to allow (which I think it doesn't as currently proposed), in particular adding a string.Format method. This also ties in with the discussion I want to have in API review around IDisposable.

@stephentoub
Copy link
Member Author

I didn't see any problems in the implementation, BTW.

Thanks for reviewing!

Also skimmed the unit tests and they looked fine. My feedback was mainly a bunch of questions.

I tried to make them exhaustive. But CoreLib code coverage is currently broken (#51058), so it's harder to validate that.

@stephentoub stephentoub marked this pull request as ready for review April 13, 2021 23:48
@stephentoub
Copy link
Member Author

Updated with @GrabYourPitchforks's feedback as well as changes from today's API review. Marked as ready for review.

@stephentoub stephentoub merged commit 516db73 into dotnet:main Apr 15, 2021
@stephentoub stephentoub deleted the interpolatedstrings branch April 15, 2021 10:22
@ladeak
Copy link
Contributor

ladeak commented Apr 15, 2021

What happens to the rented array when ToStringAndClear is not called and the InterpolatedStringBuilder goes out of scope? Could that be a source of memory leak when used incorrectly?

@stephentoub
Copy link
Member Author

Could that be a source of memory leak when used incorrectly?

No. It simply won't be pooled.

But this type is also in CompilerServices and is intended to be used by the compiler, which should use it correctly.

@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.