-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fixing assert conditions for System.Net.Security. #5642
Conversation
@@ -301,12 +301,13 @@ private int StartFrameBody(int readBytes, byte[] buffer, int offset, int count, | |||
return 0; | |||
} | |||
|
|||
if ((readBytes == _ReadHeader.Length)) | |||
if (!(readBytes == _ReadHeader.Length)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: !=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that but comparing code with Desktop becomes much harder.
A few nits, otherwise LGTM. Though most of the changes are just adding/removing blank lines... you might want to put those in a separate commit, just to help highlight the actual fixes. |
Thanks @stephentoub
I'd rather not separate them - GitHub is doing a great job in coloring the diff. |
@@ -42,6 +42,7 @@ internal unsafe SslConnectionInfo(byte[] nativeBuffer) | |||
{ | |||
GlobalLog.Assert("SslConnectionInfo::.ctor", "Negative size."); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tend to add blank lines after clauses (such as if clauses) since that was/is the StyleCop guidelines for formatting C# code. This particular formatting is not detailed in the current CoreFx guidelines. Personally, I do like it as it improves readability. We might want to consider adding this guidelines for the official CoreFx style guidelines.
LGTM |
Fixing assert conditions for System.Net.Security.
This is required for #5077. |
Fixing assert conditions for System.Net.Security. Commit migrated from dotnet/corefx@fab4772
Fixing incorrectly converted Debug.Assert during NegotiateStream port (9a414b0).
These affect DBG builds, will render incorrect ETW in production builds and impact test execution.
Tested locally by removing all ActiveIssues temporarily to ensure we get enough coverage.
@davidsh @stephentoub @shrutigarg PTAL