-
Notifications
You must be signed in to change notification settings - Fork 121
Added IPAddress
#64
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
Added IPAddress
#64
Conversation
|
||
@usableFromInline | ||
init<T: CInternetAddress>(_ cInternetAddress: T) throws { | ||
let cString = UnsafeMutablePointer<CChar>.allocate(capacity: T.stringLength) |
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.
I don’t think we need this, we could safely use String(unsafeUninitializedCapacity:initializingWith:)
.
|
||
internal func system_inet_ntop(_ family: Int32, _ pointer : UnsafeRawPointer, _ string: UnsafeMutablePointer<CChar>, _ length: UInt32) -> UnsafePointer<CChar>? { | ||
#if ENABLE_MOCKING | ||
//if mockingEnabled { return _mock(family, pointer, string, 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.
Is this commented out on purpose?
case v6(IPv6Address) | ||
} | ||
|
||
public extension IPAddress { |
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 access qualifiers for extension members aren’t being placed consistently in this patch. I don’t know what the project style is, but either they should all go on the extension members or they should all go on the extension itself.
public extension IPAddress { | ||
|
||
@_alwaysEmitIntoClient | ||
func withUnsafeBytes<Result>(_ body: ((UnsafeRawBufferPointer) -> (Result))) -> Result { |
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.
I think we should provide a mutable version of this function as well.
/// Initialize with raw bytes. | ||
@_alwaysEmitIntoClient | ||
init(_ byte0: UInt8, _ byte1: UInt8, _ byte2: UInt8, _ byte3: UInt8) { | ||
self.init(unsafeBitCast((byte0, byte1, byte2, byte3), to: CInterop.IPv4Address.self)) |
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.
I don’t think we have any reason to need unsafeBitCast
here, this can be fairly straightforwardly written as a direct initialization.
|
||
/// IPv4 Socket Address | ||
@frozen | ||
public struct IPv4Address: Equatable, Hashable, Codable { |
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.
Should we consider making this a RandomAccessCollection, MutableCollection
of UInt8
?
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.
In either case we should definitely let you construct these with a Collection where Element == UInt8
.
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.
I don't think it makes sense to make IP addresses collections of UInt8; they are technically defined as 32/128-bit identifiers. That you can slice the bits up and view, say, an IPv4 address as (4xUInt8) or (2xUInt16) or (1xUInt32) is interesting, and can make some manipulation easier, but it isn't fundamental to what an IP address is.
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.
I don't inherently disagree with that, but we either have to have a useful way to project them to a number or to bytes, otherwise getting them as a sequence of bytes is a pain in the neck. There are lots of reasons to want to be able to do that, most notably when working as network protocols, and having a safe way to do it is useful. Given that projecting an IPv6 address as an integer is not possible in Swift without swift-numerics (an unreasonable dependency for this project), I'm inclined to want to still offer it.
|
||
/// Initialize with bytes | ||
@_alwaysEmitIntoClient | ||
init(_ byte0: UInt16, _ byte1: UInt16, _ byte2: UInt16, _ byte3: UInt16, _ byte4: UInt16, _ byte5: UInt16, _ byte6: UInt16, _ byte7: UInt16) { |
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.
UInt16s aren’t bytes.
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.
They could be - "byte" is an ambiguous term. This is why internet standards (and WebURL's IP-address types) use the term "octet". When slicing a 128-bit IPv6 address in to 16-bit units, the terms I've seen most often are "segments" and "pieces" (WebURL uses the latter).
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.
This is technically true, but I'm going to argue that it's the least useful kind of technically true. The number of developers who see the word "byte" and think "8 bits" overwhelmingly outnumbers the number of people who see the word "byte" and think "I don't know how big this is". Calling these bytes invites confusion (I guarantee that some time down the road we'll get a developer wanting to reword this initializer if we don't do it ourselves), so we can just not use that word.
So a couple of things:
Overall, I'm -1 on adding IP addresses to this library. There are important use-cases which need to talk about IP addresses but don't require a full port of swift-system. I would prefer that we go with @Lukasa's suggestion from the previous PR and create a lower-level library of platform-independent networking types and operations and which swift-system could use. |
This is pretty speculative, I just copied the `SwiftSupport.cmake` from swift-argument-parser which seems to have support for arm64.
Added
IPAddress
enum withIPv4Address
,IPv6Address
structs.