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
| -- 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?
There was a problem hiding this comment.
Hmm I see. I asked because this macro is being defined in the cabal file just based on windows. If the macro is intended to check if the compiler supports WINIO then GHC already sets __IO_MANAGER_WINIO__ so perhaps that's better to use rather than a custom macro?
There was a problem hiding this comment.
Other than that the rest look good
There was a problem hiding this comment.
Oh, do you want to get rid of HAS_WINIO throughout and replace with __IO_MANAGER_WINIO__? That sounds like a good idea.
There was a problem hiding this comment.
Yeah, that's the recommended macro, because it let's the compiler stay in control!
We might be able to leverage this macro to tell which compilers have a "fixed" winio for network, if we eg make the macro a version. So like with your ghc patch (I gave some comments btw) we can change the macro to be, say version 2 and check for that instead?
2 participants
This contains all the work we've been doing in #602.
CC @kazu-yamamoto