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.

Sign up for GitHub

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

Jump to bottom

Annotate System.Net.Sockets for nullable reference types #32675

Merged
merged 17 commits into from
Feb 27, 2020
Merged

Annotate System.Net.Sockets for nullable reference types #32675

merged 17 commits into from
Feb 27, 2020

Conversation

eerhardt
Copy link
Member

Contributes to #2339

Depends on #32518.

/cc @dotnet/ncl

@danmoseley
Copy link
Member

danmoseley commented Feb 22, 2020 •
edited by eerhardt
Loading

Please help add exactly one area label if the bot does not. It teaches the bot. #Closed

@eerhardt
Copy link
Member Author

eerhardt commented Feb 24, 2020 •
edited
Loading

Please help add exactly one area label if the bot does not. It teaches the bot.

Any thoughts on having the bot post a comment when it doesn't add a label? It could mention that it couldn't add a label, maybe log which areas it thinks are the closest, and ask a human to manually add a label. #Closed

@eerhardt eerhardt mentioned this pull request Feb 24, 2020
Open
@eerhardt
Copy link
Member Author

eerhardt commented Feb 26, 2020 •
edited
Loading

@eiriktsarpalis @buyaa-n - any feedback? #Closed

stephentoub
stephentoub reviewed Feb 26, 2020
View reviewed changes
src/libraries/Common/src/Interop/Windows/WinSock/Interop.TransmitFile.cs
@@ -14,7 +14,7 @@ internal static partial class Mswsock
[DllImport(Interop.Libraries.Mswsock, SetLastError = true)]
internal static extern unsafe bool TransmitFile(
SafeHandle socket,
SafeHandle fileHandle,
SafeHandle? fileHandle,
Copy link
Member

@stephentoub stephentoub Feb 26, 2020 •
edited by eerhardt
Loading

Choose a reason for hiding this comment

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

In what situation do we pass null here? #Closed

Copy link
Member Author

@eerhardt eerhardt Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In the case where BeginSendFile(string? fileName, byte[]? preBuffer, byte[]? postBuffer is called with a null or empty fileName. #Closed

stephentoub
stephentoub approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Skimmed through it and left a few comments, but generally LGTM. Welcome to the wonderful world of nullable reference types ;)

buyaa-n
buyaa-n approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Thanks!

@eerhardt
Copy link
Member Author

Thanks for all the feedback. I have addressed it, and if I don't receive anymore I'll merge this when it is green.

@eerhardt
Copy link
Member Author

The 2 OSX failures are #32882.

@eerhardt eerhardt merged commit e22cf55 into dotnet:master Feb 27, 2020
@eerhardt eerhardt deleted the AnnotateNetSockets branch February 27, 2020 03:19
@antonfirsov antonfirsov mentioned this pull request Feb 27, 2020
Merged
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@eerhardt eerhardt mentioned this pull request Sep 23, 2020
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@buyaa-n buyaa-n buyaa-n approved these changes

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

Assignees
No one assigned
Labels
area-System.Net.Sockets
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@eerhardt @danmoseley @stephentoub @eiriktsarpalis @buyaa-n @karelz