Open pedro823 opened 1 year ago
Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.
Author: | pedro823 |
---|---|
Assignees: | - |
Labels: | `api-suggestion`, `area-System.Net` |
Milestone: | - |
Potentially duplicate of #56627. Is there any difference after IPNetwork
is introduced?
You can pass a custom IComparer
for the cases of Array.BinarySearch
and List.Sort
.
You can pass a custom IComparer for the cases of Array.BinarySearch and List.Sort.
Yes, however implementing an IComparer
for IPAddress
efficiently would require access to the underlying uint
or ushort[]
inside it.
This is the best comparer I could do for this demo, and it has some arbitrary decisions which work in my case:
// NOT HEAVILY TESTED - DO NOT USE IN PRODUCTION :)
public class IPComparer : IComparer<IPAddress>
{
[Pure]
public int Compare(IPAddress? a, IPAddress? b)
{
if (a is null || b is null)
{
if (a is null && b is null)
{
return 0;
}
return a is null ? -1 : 1;
}
// two IPv6 IPs would use at most 32 bytes in the stack.
Span<byte> scratchArea = stackalloc byte[32];
// Should always work.
_ = a.TryWriteBytes(scratchArea, out var aLength);
_ = b.TryWriteBytes(scratchArea[aLength..], out var bLength);
return Compare(scratchArea[..aLength], scratchArea[aLength..(aLength + bLength)]);
}
[Pure]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int Compare(Span<byte> a, Span<byte> b)
{
// HACK: An IPv4 should be translated to IPv6 before
// comparison. We have RFC6502 describing how to
// do that translation.
if (a.Length != b.Length)
{
return a.Length.CompareTo(b.Length);
}
for (var i = 0; i < a.Length; i++)
{
if (a[i] != b[i])
return a[i] - b[i];
}
return 0;
}
}
As you can see, the client needs to extract the underlying integer representation of the IPAddress to be able to compare them.
The issue you mentioned has this comment:
It seems really weird to add IComparable
in these cases, since there is no well defined ordering and so whatever we did would be arbitrary.
However, is it arbitrary to compare IPAddresses as the uint
they are? Or in the v6 case, as the uint128
they are? In the case of mixing both of them, we have an RFC (RFC6502) which well defines the translation between IPv4 and IPv6 -- we can translate one to the other and compare them in the IPv6 realm.
metaquestion: should we continue the discussion here or move to the original issue #56627?
metaquestion: should we continue the discussion here or move to the original issue #56627?
The old issue is locked. Use new issue to discuss the differences.
I can submit a PR to implement IEquatable<IPAddress>
, IComparable<IPAddress>
(and perhaps the non-generic IComparable
for completeness' sake, although I don't think it'll do much) if the API proposal's approved. Implementing IEquatable also eliminates a cast in the default equality comparer, although I'm not sure how meaningful of a performance impact that'd actually have.
It might also be a good idea to add the comparison operators. This could turn the API proposal into something similar to the below:
public class IPAddress : ISpanFormattable, ISpanParsable<IPAddress>, IUtf8SpanFormattable
+ , IEquatable<IPAddress>, IComparable<IPAddress>, IComparable
{
+ public bool Equals(IPAddress? other);
+ int IComparable<IPAddress>.CompareTo(IPAddress? other);
+ int IComparable.CompareTo(object? obj)
+ public static bool operator <(IPAddress? left, IPAddress? right);
+ public static bool operator >(IPAddress? left, IPAddress? right);
+ public static bool operator <=(IPAddress? left, IPAddress? right);
+ public static bool operator >=(IPAddress? left, IPAddress? right);
}
Background and motivation
Currently, there's no way of sorting an
IPAddress[]
without doing some janky or un-performatic code to implementIComparable<IPAddress>
.The best way to implement
IComparable<IPAddress>
today would rely on theGetAddressBytes()
method (which allocates abyte[]
) or on theTryWriteBytes(Span<byte> destination, out int bytesWritten)
method (which makes the implementation really annoying).If, instead, IPAddress implemented
IComparable<IPAddress>
, we'd be able to have zero code in the client's side and would not require any allocations either.Real world use: I'm implementing software which requires to check whether an IP belongs to a big set of CIDRs. Currently, my approach's algorithm guarantees that those CIDRs are disjointed, making the implementation be a binary search over the
IPNetwork
's starting IPAddress, and then the.Contains()
method of theIPNetwork
class. BecauseIPAddress
lacks a comparator, there's no good way of implementing this binary search.API Proposal
Adding
IEquatable
andIComparable
toIPAddress
's declaration:API Usage
Alternative Designs
No response
Risks
No response