-
Notifications
You must be signed in to change notification settings - Fork 48
Conversation
@@ -200,14 +200,14 @@ internal class PoCSocket { | |||
#if os(Linux) | |||
var addr = sockaddr_in( | |||
sin_family: sa_family_t(AF_INET), | |||
sin_port: UInt16(port), | |||
sin_port: htons(UInt16(port)), |
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 you can use UInt16(bigEndian:)
(and below).
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.
@weissi is this the same thing as you did in swiftlang/swift-corelibs-foundation#1185 ?
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 use of htons()
on Linux for host to network byte order conversion is pretty standard - and probably makes it clearer what the code is doing here. The non-Linux case is more opaque.
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.
@ianpartridge do you have any major concerns merging as-is? We can always revisit htons
vs. alternatives later.
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.
No major concerns, I was suggesting a possible improvement which we can always make in future.
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.
@ianpartridge / @seabaylea htons()
is the right thing to use here. The only UInt16(bigEndian:)
is the wrong way around (usually called ntohs()
). The only reason I used it and not ntohs()
is that ntohs
is a C macro that's not imported into Swift.
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.
you could use UInt16(port).bigEndian
but htons()
is to be preferred as it IMHO carries more semantic meaning.
Ok - merging as-is and raising an issue to look at |
Raised #44. |
Setting the port for the HTTPServer() was being ignore, both because we weren't passing the port down to the underlying server and socket, and because PoCSocket wasn't carrying out the host to network byte order conversion on Darwin.