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

Pass interpolated string handlers by ref #57538

Closed
stephentoub opened this issue Aug 17, 2021 · 2 comments · Fixed by #57536
Closed

Pass interpolated string handlers by ref #57538

stephentoub opened this issue Aug 17, 2021 · 2 comments · Fixed by #57536
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@stephentoub
Copy link
Member

Background and motivation

Our default interpolated string handler, StringBuilder handler, and MemoryExtensions handler are all passed to their target methods by ref. But we neglected to do so for the Debug handlers; we should update those signatures as well.

Benefits:

  • Consistency across all our handlers
  • These are typically larger structs and so by ref helps (though it doesn’t matter for Debug)
  • Making it a ref makes it even less attractive to try to call directly / makes it look even more special
  • It gives us the opportunity to more safely clean up at the end of the operation

API Proposal

See the ref updates here:
https://github.com/dotnet/runtime/pull/57536/files#diff-cec8e6f471b4193246bdc0107b0dd7cbe131fb7fd189b288b37269c333d1171d

API Usage

No usage difference when using interpolated string syntax, which is how we expect these APIs to be used.

Risks

This is a breaking change from .NET 6 Preview 7.

@stephentoub stephentoub added area-System.Runtime blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 17, 2021
@stephentoub stephentoub added this to the 6.0.0 milestone Aug 17, 2021
@ghost
Copy link

ghost commented Aug 17, 2021

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

Issue Details

Background and motivation

Our default interpolated string handler, StringBuilder handler, and MemoryExtensions handler are all passed to their target methods by ref. But we neglected to do so for the Debug handlers; we should update those signatures as well.

Benefits:

  • Consistency across all our handlers
  • These are typically larger structs and so by ref helps (though it doesn’t matter for Debug)
  • Making it a ref makes it even less attractive to try to call directly / makes it look even more special
  • It gives us the opportunity to more safely clean up at the end of the operation

API Proposal

See the ref updates here:
https://github.com/dotnet/runtime/pull/57536/files#diff-cec8e6f471b4193246bdc0107b0dd7cbe131fb7fd189b288b37269c333d1171d

API Usage

No usage difference when using interpolated string syntax, which is how we expect these APIs to be used.

Risks

This is a breaking change from .NET 6 Preview 7.

Author: stephentoub
Assignees: -
Labels:

area-System.Runtime, blocking, api-ready-for-review

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 17, 2021
@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Aug 17, 2021
@stephentoub stephentoub self-assigned this Aug 17, 2021
@stephentoub stephentoub added the in-pr There is an active PR which will close this issue when it is merged label Aug 17, 2021
@bartonjs
Copy link
Member

bartonjs commented Aug 17, 2021

Video

Looks good as proposed. Hopefully we'll be diligent in the future at always using pass-by-ref when the handler is a struct.

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 17, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 17, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants