Skip to content

Fixer: Prefer Memory overloads for Stream async Read/Write methods#3592

Merged
carlossanlop merged 17 commits intodotnet:masterfrom
carlossanlop:PreferStreamAsyncMemoryOverloads
May 7, 2020
Merged

Fixer: Prefer Memory overloads for Stream async Read/Write methods#3592
carlossanlop merged 17 commits intodotnet:masterfrom
carlossanlop:PreferStreamAsyncMemoryOverloads

Conversation

@carlossanlop
Copy link

@carlossanlop carlossanlop commented May 1, 2020

Fixes dotnet/runtime#33790

Summary

If the user invokes any of these methods from an object that is or inherits from Stream:

ReadAsync(byte[] buffer, int offset, int count)
ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
WriteAsync(byte[] buffer, int offset, int count)
WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)

We should suggest to instead use:

ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken)
WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken)

Works for both C# and VB.
All unit tests are passing locally.

@carlossanlop carlossanlop requested a review from a team as a code owner May 1, 2020 00:08
@carlossanlop carlossanlop self-assigned this May 1, 2020
@carlossanlop carlossanlop requested a review from jeffhandley May 1, 2020 00:08
@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #3592 into master will decrease coverage by 0.00%.
The diff coverage is 98.21%.

@@            Coverage Diff             @@
##           master    #3592      +/-   ##
==========================================
- Coverage   95.15%   95.15%   -0.01%     
==========================================
  Files        1069     1070       +1     
  Lines      242339   242302      -37     
  Branches    15753    15765      +12     
==========================================
- Hits       230596   230553      -43     
- Misses      10000    10002       +2     
- Partials     1743     1747       +4

@codecov
Copy link

codecov bot commented May 4, 2020 •
edited
Loading

Codecov Report

Merging #3592 into master will increase coverage by 0.00%.
The diff coverage is 97.32%.

@@           Coverage Diff            @@
##           master    #3592    +/-   ##
========================================
  Coverage   95.15%   95.16%            
========================================
  Files        1069     1071     +2     
  Lines      242339   243061   +722     
  Branches    15753    15789    +36     
========================================
+ Hits       230596   231305   +709     
- Misses      10000    10003     +3     
- Partials     1743     1753    +10

@carlossanlop
Copy link
Author

carlossanlop commented May 4, 2020

I also need to resolve @mavasani 's request to group the description and title messages into one: #3586

mavasani
mavasani reviewed May 6, 2020
View reviewed changes
Copy link

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. Thanks for extensive tests. I should be good to sign off once the last set of suggestions are addressed.

@carlossanlop carlossanlop requested review from jozkee and mavasani May 6, 2020 23:36
mavasani
mavasani approved these changes May 7, 2020
View reviewed changes
Copy link

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

LGTM. I have couple more suggestions:

  1. Use new MyCodeAction as MyCodeAction.Create will do the same thing as CodeAction.Create
  2. Use localized title string without method name as the first argument for title.

@carlossanlop
Copy link
Author

carlossanlop commented May 7, 2020

LGTM. I have couple more suggestions:

  1. Use new MyCodeAction as MyCodeAction.Create will do the same thing as CodeAction.Create
  2. Use localized title string without method name as the first argument for title.

Fixed, @mavasani. Thanks. I'll merge as soon as the build finishes.

@carlossanlop carlossanlop merged commit 63f9aff into dotnet:master May 7, 2020
@pgovind pgovind mentioned this pull request May 8, 2020
Merged
@carlossanlop carlossanlop mentioned this pull request May 11, 2020
Closed
@carlossanlop carlossanlop deleted the PreferStreamAsyncMemoryOverloads branch May 14, 2020 21:08
@carlossanlop carlossanlop mentioned this pull request Jan 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stephentoub stephentoub stephentoub left review comments

@pgovind pgovind Awaiting requested review from pgovind

@bartonjs bartonjs Awaiting requested review from bartonjs

@buyaa-n buyaa-n Awaiting requested review from buyaa-n

@jeffhandley jeffhandley Awaiting requested review from jeffhandley

@jozkee jozkee Awaiting requested review from jozkee

+1 more reviewer

@mavasani mavasani mavasani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlop carlossanlop

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Prefer Memory<T> overloads for Stream.Read/WriteAsync

4 participants

@carlossanlop @stephentoub @mavasani @jozkee