Conversation
|
Right, had a quick look and so far looks good. I'll have a deeper review thursday morning. I do like this approach because it also fixes the type error on the POSIX path on Windows that's been long standing. |
|
This looks really good! I have some nits and the build errors need to be fixed. |
Mistuke
left a comment
Mistuke
left a comment
There was a problem hiding this comment.
Oops forgot to publish the review comments
Network/Socket/Buffer.hsc
Outdated
| recvBufFromMIO s ptr nbytes | ||
| # endif | ||
| #else | ||
| withNewSocketAddress $ \ptr_sa sz -> alloca $ \ptr_len -> |
There was a problem hiding this comment.
Is this not the same as the recvBufFromMIO below? so couldn't we just reuse one of them?
Network/Socket/Buffer.hsc
Outdated
| -- If the control buffer was truncated (MSG_CTRUNC), the | ||
| -- control data may be invalid and parsing could segfault. | ||
| cmsgs <- if msgCtrl hdr == nullPtr || (rawFlags .&. #{const MSG_CTRUNC}) /= 0 | ||
| then return [] | ||
| else parseCmsgs msgHdrPtr |
There was a problem hiding this comment.
This looks like a separate bugfix. Perhaps move it to a separate commit and new changelog entry?
There was a problem hiding this comment.
I'm not sure why, but the Windows tests weren't passing without this. Maybe using WinIO exposed a new behavior here, so it's connected to WinIO. But I can split it out if you like.
| -- Since 2.7.0.0. | ||
| getNonBlock :: CInt -> IO Bool | ||
| getNonBlock :: CSocket -> IO Bool | ||
| #if defined(mingw32_HOST_OS) |
There was a problem hiding this comment.
think this can be dropped.
There was a problem hiding this comment.
You mean remove the #if defined(mingw32_HOST_OS) ?
I don't think we can, since there are separate code paths here for Windows with WinIO, Windows without WinIO, and non-Windows. The first two are different because we always conditionally import <!> gated on HAS_WINIO. But maybe I'm misunderstanding you?
2 participants
This contains all the work we've been doing in #602.
CC @kazu-yamamoto