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 async overloads for ByteString.WriteTo() #6228

Conversation

erikmav
Copy link
Contributor

@erikmav erikmav commented Jun 7, 2019

Related to #3166

@erikmav erikmav changed the title Add async overload for ByteString.WriteTo() Add async overloads for ByteString.WriteTo() Jun 7, 2019
@acozzette
Copy link
Member

CC @jskeet @anandolee

@jtattermusch Would you be interested in reviewing this? @anandolee usually reviews C# changes but is on vacation for the next month.

@erikmav
Copy link
Contributor Author

erikmav commented Jun 11, 2019

@acozzette Would you mind adding the "C#" tag here and in my other PR? The mergeable-bot fails on that missing label, and I can't add labels

@acozzette acozzette added the c# label Jun 11, 2019
@jtattermusch
Copy link
Contributor

Same as #6230 (comment) can you describe the use case?
Also, on netstandard2.0 you can use the .Span property and write to an output stream (or to a pipewriter) in a more flexible fashion.

@erikmav
Copy link
Contributor Author

erikmav commented Jun 11, 2019

Good point and I'll move toward that. In this case I'm working in a net472 codebase, the .Span property is not visible in this case, so I was optimizing for what was available to minimize thread waits.

@erikmav erikmav closed this Jun 11, 2019
@erikmav erikmav reopened this Jun 14, 2019
@erikmav
Copy link
Contributor Author

erikmav commented Jun 14, 2019

@jtattermusch Memory only supported on netcore2.1+. I'd prefer to get this change in for downlevel net472 optimization.

@BSBandme BSBandme requested review from jtattermusch and removed request for jtattermusch June 20, 2019 02:42
@jtattermusch
Copy link
Contributor

Same response as on #6230 (comment)

@erikmav
Copy link
Contributor Author

erikmav commented Aug 14, 2019

@jtattermusch This is different from #6230 as it simply adds the ability to write to a stream without holding a thread context. I closed that PR, but this one still seems useful by itself.

@jtattermusch jtattermusch self-assigned this Aug 29, 2019
@jtattermusch jtattermusch self-requested a review August 29, 2019 13:09
@jtattermusch
Copy link
Contributor

jtattermusch commented Aug 29, 2019

Once we add the System.Memory dependency for all targets (see #6317), it seems that this will no longer be useful, because you can grab byteString.Span and use that to write to the Stream? If that's the case, let's close because then there wouldn't be much point in adding new APIs.

@erikmav erikmav closed this Aug 29, 2019
@erikmav erikmav deleted the dev/erikmav/byteStringWriteToAsync branch May 19, 2022 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants