-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Add IPNetwork.Parse and TryParse #44573
Conversation
Thanks for your PR, @david-acker. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
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.
Left some nits / hints.
} | ||
|
||
if (prefix.AddressFamily == AddressFamily.InterNetwork && prefixLength > 32) | ||
{ | ||
throw new ArgumentOutOfRangeException(nameof(prefixLength)); | ||
return false; | ||
} | ||
|
||
if (prefix.AddressFamily == AddressFamily.InterNetworkV6 && prefixLength > 128) |
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.
Unrleated to your change: but these should be if-else if-else
to avoid double-checks, or actually I'd write it as
static bool IsValidPrefixLengthRange1(IPAddress prefix, int prefixLength)
{
return prefix.AddressFamily switch
{
AddressFamily.InterNetwork => prefixLength <= 32,
AddressFamily.InterNetworkV6 => prefixLength <= 128,
_ => prefixLength >= 0
};
}
and let Roslyn and JIT optimize it.
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.
Ah, that's a good point. Although with your example, the prefixLength >= 0
check isn't be enforced for the non-default cases so negative prefixLength
values would slip through. How about this instead?
static bool IsValidPrefixLengthRange(IPAddress prefix, int prefixLength)
{
+ if (prefixLength < 0)
+ {
+ return false;
+ }
return prefix.AddressFamily switch
{
AddressFamily.InterNetwork => prefixLength <= 32,
AddressFamily.InterNetworkV6 => prefixLength <= 128,
- _ => prefixLength >= 0
+ _ => true
};
}
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.
Yep, you're right 👍🏻
@MihaZupan this type seems like something you'd expect in System.Net. I wonder whether there is missing API there. |
We've had a few issues about CIDR support in It looks like users are satisfied with the NuGet package that offers all sorts of these helpers https://github.com/jsakamoto/ipaddressrange and haven't been pushing on the runtime team. |
There's no package out there that I'm completely happy with. I'd like something in I rolled my own version of this last week to avoid adding a dependency to my library. |
If you want to pursue that, you'll want to create an API proposal there of course. Edit: oh, I misread, sounds like we already have discussion there. |
@david-acker, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work! Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers. This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement. Thanks! |
Thanks |
Add Parse and TryParse methods for IPNetwork
Adds
Parse
andTryParse
methods for creating anIPNetwork
using an input string in CIDR notation or 'slash notation'Description
Adds the following
Parse
andTryParse
static methods toIPNetwork
:Examples
IPv4
IPv6
Fixes #8606