Unify MSVC and MinGW-w64 code paths #12769
Conversation
Changes
Outdated
| @@ -178,6 +178,10 @@ Working version | |||
| Updates usage of sigprocmask to pthread_sigmask in otherlibs/unix. | |||
| (Max Slater, review by Miod Vallat and Xavier Leroy) | |||
|
|
|||
| - #?????: Unify MSVC and MinGW-w64 code paths, by always using WinAPI | |||
| directly. | |||
| (David Allsopp, Antonin Décimo, and Samuel Hym, review by ????) | |||
There was a problem hiding this comment.
The wording of this changelog entry seems to imply that no other places remain in the code with separate MinGW/MSVC codepaths? Is that the case? If not, I would suggest being more precise (eg mention that this is just about usleep/Sleep or write "Unify some MSVC and MinGW-w64 code paths, ...".
There was a problem hiding this comment.
Well, we were quite surprised that it seems to be the only place in the runtime where there were/could have been different code paths involving <unistd.h>.
| @@ -18,8 +18,11 @@ | |||
|
|
|||
| #define CAML_INTERNALS | |||
|
|
|||
| #include "caml/config.h" | |||
There was a problem hiding this comment.
Can you say a few words about why this included was added?
There was a problem hiding this comment.
It's where HAS_UNISTD is defined. If no caml/*.h header is included, as they end up including caml/config.h too, then we don't get this macro. From the Autoconf manual:
The package should ‘
#include’ the configuration header file before any other header files, to prevent inconsistencies in declarations (for example, if it redefinesconst, or if it defines a macro like_FILE_OFFSET_BITSthat affects the behavior of system headers).
https://www.gnu.org/software/autoconf/manual/autoconf-2.71/html_node/Configuration-Headers.html
There was a problem hiding this comment.
I've added a short note about this in the commit message.
|
(The Changes entry needs to be updated.) |
Co-authored-by: Samuel Hym <samuel.hym@rustyne.lautre.net>
Avoid the MinGW implementation of this library so that the MinGW and MSVC ports use the same WinAPI code. Don't define HAS_UNISTD under Windows hosts, always guard its inclusion with this macro, defined in "caml/config.h". Co-authored-by: Antonin Décimo <antonin@tarides.com> Co-authored-by: Samuel Hym <samuel.hym@rustyne.lautre.net>
MisterDA
force-pushed
the
windows-no-unistd
branch
from
db41a65
to
2d4d332
Compare
|
Thanks for the review! I've updated the changes entry. |
MinGW-w64 emulates functions from
<unistd.h>using the WinAPI. The MSVC port doesn't provide this header, and we must use the WinAPI directly. In order to have one code path shared by the two ports, we never include<unistd.h>with MinGW-w64 and always use the WinAPI directly in the runtime.MinGW-w64 emulates
usleep(µs-resolution) withSleep(ms-resolution). We simply callSleepdirectly. We have a follow-up plan for higher-resolution sleeps on Windows.We have checked that in all files used on Windows,
unistd.his guarded byHAS_UNISTD. This can be tested again withgrep -R -B1 '<unistd\.h>'.This goes towards restoring the MSVC port.