Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fixing assert conditions for System.Net.Security. #5642

Merged
merged 1 commit into from
Jan 25, 2016

Conversation

CIPop
Copy link

@CIPop CIPop commented Jan 23, 2016

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

@CIPop CIPop added test bug Problem in test source code (most likely) area-System.Net labels Jan 23, 2016
@CIPop CIPop self-assigned this Jan 23, 2016
@CIPop CIPop added this to the 1.0.0-rc2 milestone Jan 23, 2016
@@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: !=

Copy link
Author

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.

@stephentoub
Copy link
Member

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.

@CIPop
Copy link
Author

CIPop commented Jan 23, 2016

Thanks @stephentoub

you might want to put those in a separate commit

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.");
}

Copy link
Contributor

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.

@davidsh
Copy link
Contributor

davidsh commented Jan 23, 2016

LGTM

CIPop added a commit that referenced this pull request Jan 25, 2016
Fixing assert conditions for System.Net.Security.
@CIPop CIPop merged commit fab4772 into dotnet:master Jan 25, 2016
@CIPop CIPop deleted the snsasserts1 branch January 25, 2016 18:40
@CIPop
Copy link
Author

CIPop commented Jan 25, 2016

This is required for #5077.

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Fixing assert conditions for System.Net.Security.

Commit migrated from dotnet/corefx@fab4772
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net test bug Problem in test source code (most likely)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants