Add support for Windows#869
Conversation
|
| return (a=='M'&&b=='o'&&c=='n')||(a=='T'&&b=='u'&&c=='e')||(a=='W'&&b=='e'&&c=='d')|| | ||
| (a=='T'&&b=='h'&&c=='u')||(a=='F'&&b=='r'&&c=='i')||(a=='S'&&b=='a'&&c=='t')|| | ||
| (a=='S'&&b=='u'&&c=='n'); |
There was a problem hiding this comment.
This is a little hard to parse without any whitespace. It'd probably benefit from having one day one per line.
| .serverChannelOption(ChannelOptions.socketOption(.so_reuseaddr), value: 1) | ||
| .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEPORT), value: 1) | ||
| #if !os(Windows) | ||
| .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEPORT), value: 1) |
There was a problem hiding this comment.
formatting looks off here
| // Check for valid weekday abbreviation (Mon..Sun) | ||
| // Expect exactly 3 ASCII letters. | ||
| if (!p) return 0; | ||
| char a = p[0], b = p[1], c = p[2]; |
There was a problem hiding this comment.
We're potentially reading out of bounds here.
| int hh = parse_fixed2(pp); if (hh < 0) return 0; | ||
| if (!expect_char(pp, ':')) return 0; | ||
| int mm = parse_fixed2(pp); if (mm < 0) return 0; | ||
| if (!expect_char(pp, ':')) return 0; | ||
| int ss = parse_fixed2(pp); if (ss < 0) return 0; | ||
| if (hh > 23 || mm > 59 || ss > 60) return 0; // allow leap second 60 | ||
| *h = hh; *m = mm; *s = ss; |
There was a problem hiding this comment.
can you avoid multiple statements per line? It generally just makes readability worse.
| server = try ServerBootstrap(group: group) | ||
| .serverChannelOption(ChannelOptions.socket(.init(SOL_SOCKET), .init(SO_REUSEADDR)), value: 1) | ||
| #if !os(Windows) | ||
| .serverChannelOption(ChannelOptions.socket(.init(SOL_SOCKET), .init(SO_REUSEADDR)), value: 1) |
There was a problem hiding this comment.
Formatting looks off here (and below)
| #else | ||
| nonisolated(unsafe) private let posixLocale: UnsafeMutableRawPointer = { | ||
| // FIXME: This can be cleaner. But the Windows shim doesn't need a locale pointer | ||
| UnsafeMutableRawPointer(bitPattern: 0)! |
There was a problem hiding this comment.
This init is documented to return nil for a bitPattern of 0 so this is a crash in disguise. If the Windows shim doesn't need a locale pointer, can we just remove this branch entirely and update the caller?
| return timestamp == -1 && errno == EOVERFLOW ? nil : timestamp | ||
|
|
||
| #if os(Windows) | ||
| let err = GetLastError() |
There was a problem hiding this comment.
_mkgmtime is a CRT function, doesn't that mean you should be checking errno? (Apologies if that's a naive Windows question...)
| XCTAssertEqual("abc\"", c?.value) | ||
| } | ||
|
|
||
| #if !os(Windows) |
There was a problem hiding this comment.
Can you move this inside the text and do an XCTSkip for Windows? That way we can document why the test is skipped.
| // _mktemp_s is not great, as it's rumored to have limited randomness, but Windows doesn't have mkstemp | ||
| // And this is a test utility only. |
There was a problem hiding this comment.
There's no rumor about this:
_mktemp_scan create a maximum of 26 unique file names for any given combination of base and nameTemplate values.
From the docs: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/mktemp-s-wmktemp-s?view=msvc-170
This is such a limitation that I wonder if tests might legitimately hit this.
3 participants
TODOs
I'm thinking of replacing timestamp parsing with Foundation for now, or at least writing an implementation in Swift. I wanted to check what folks prefer for now.