-
Couldn't load subscription status.
- Fork 5.2k
[NET] IPNetwork clears the lower bits instead of throwing. #117367
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.
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
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
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.
Pull Request Overview
This PR changes the IPNetwork implementation to automatically clear bits outside the network prefix instead of throwing an exception, and updates tests to verify the new behavior.
- IPNetwork constructor and TryParse now use
ClearNonZeroBitsAfterNetworkPrefixto trim bits rather than rejecting the input. - The obsolete
HasNonZeroBitsAfterNetworkPrefixlogic and related error resource were removed. - Tests were updated to compute expected base addresses, moved previously invalid cases into valid data, and corrected a typo in
prefixLength.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Net.Primitives/tests/FunctionalTests/IPNetworkTest.cs | Updated valid/invalid data, added helper methods to calculate expected base addresses |
| src/libraries/System.Net.Primitives/src/System/Net/IPNetwork.cs | Replaced non-zero-bit check with clearing logic, simplified TryParse |
| src/libraries/System.Net.Primitives/src/Resources/Strings.resx | Removed obsolete error message for invalid base addresses |
Comments suppressed due to low confidence (3)
src/libraries/System.Net.Primitives/tests/FunctionalTests/IPNetworkTest.cs:39
- Add a test case for the IPv6 /0 network (e.g.
{ "::/0" }) to ensure the new zero-prefix branch for IPv6 is covered.
{ "192.168.0.10/0" },
src/libraries/System.Net.Primitives/src/System/Net/IPNetwork.cs:240
- [nitpick] Consider renaming
ClearNonZeroBitsAfterNetworkPrefixto something likeComputeNetworkAddressorTrimToNetworkPrefixto better reflect that this method returns the trimmed base address.
private static IPAddress ClearNonZeroBitsAfterNetworkPrefix(IPAddress baseAddress, int prefixLength)
src/libraries/System.Net.Primitives/src/System/Net/IPNetwork.cs:261
- [nitpick] This comment is a bit unclear—consider clarifying or removing it, since the reasoning about shift width may not be obvious to future readers.
// Bitwise shift works only for lower 7-bits count operands.
src/libraries/System.Net.Primitives/src/System/Net/IPNetwork.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Primitives/src/System/Net/IPNetwork.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Miha Zupan <[email protected]>
Co-authored-by: Miha Zupan <[email protected]>
| prefixLength <= GetMaxPrefixLength(address)) | ||
| { | ||
| Debug.Assert(prefixLength >= 0); // Parsing with NumberStyles.None should ensure that prefixLength is always non-negative. | ||
| result = new IPNetwork(address, prefixLength, false); |
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.
The baseAddress is the original non-mask address. Is it intended?
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.
Fudge, good catch. It also means we don't have a good test coverage for this. I'll put up a PR asap.
Fixes #117114