Skip to content

Conversation

@stephentoub
Copy link
Member

@Dotnet-GitSync-Bot
Copy link
Collaborator

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.

Copy link
Contributor

@geoffkizer geoffkizer left a comment

Choose a reason for hiding this comment

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

1500 fewer lines of code, nice :)

Stream.Null was using BlockingBegin/EndRead/Write, but the operations are nops (they don't even do argument validation), so the functionality was unnecessary complication.  We can just use TaskToApm instead, with already completed tasks, and then also delete the SynchronousAsyncResult that was used by these operations.

Also, the Begin/EndRead/Write methods were checking CanRead/Write and throwing if they return false, but this is a private sealed type with CanRead/Write hardcoded to return true! Delete.

Finally added consistency across Stream.Null to checking for cancellation.
Also consolidate a few resources and use a few existing ThrowHelpers where appropriate.
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

The changes look good, thank you for the PR.
I left some questions asking for clarification of some of the changes.

@stephentoub stephentoub merged commit a112379 into dotnet:master Oct 30, 2020
@stephentoub stephentoub deleted the streamargvalidation branch October 30, 2020 09:50
@ghost ghost locked as resolved and limited conversation to collaborators Dec 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Add Stream.ValidateBufferArguments helper

6 participants