search

mirage / checkseum

MIT License
15 stars 13 forks source link

Use canonical _WIN32 macro #61

Closed jonahbeckford closed 2 years ago

jonahbeckford commented 2 years ago

This PR switches from WIN32 to _WIN32 because, at minimum, WIN32 is not defined with MSVC compilers.

According to https://reviews.llvm.org/D40285 the _WIN32 macro works across MSVC, GCC and clang compilers:

In MSVC, WIN32 and WIN64 are never defined by the compiler, neither by system headers. Project files created by the IDE often contains them set manually though.

GCC on the other hand predefines both _WIN32 and WIN32 (and similarly for -64), but only when using the GNU standards additions (which are enabled by default) x86_64-w64-mingw32-gcc -E -dM - < /dev/null | grep WIN32 does include both, while the unprefixed one vanishes if you add e.g. -std=c99 (but are still included if you set -std=gnu99).

clang on the other hand doesn't check the standards version, but provides both WIN32 and _WIN32. And for the really inconsistent case, with clang -target x86_64-w64-mingw32 -E -dM - < /dev/null, you will have WIN64, _WIN64 and _WIN32, but no unprefixed WIN32.

So WIN32 is incorrect, even on GCC compilers.

The _WIN32 macro is documented at https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170

jonahbeckford commented 2 years ago

Testing

The GitHub Actions Windows target: https://github.com/jonahbeckford/checkseum/actions/runs/1965670821

samoht commented 2 years ago

Nice! Do you mind opening another PR to switch the CI from appveyor (😱 ) to GHA?

jonahbeckford commented 2 years ago

Nice! Do you mind opening another PR to switch the CI from appveyor (😱 ) to GHA?

Hmm. I have never used appveyor. It seems like appveyor.xml has already been deleted and the GitHub Actions already work. So it seems like a problem with the AppVeyor service itself still triggering on every commit.

I came across https://github.com/appveyor/ci/issues/3585 which has some comments about a) deleting the GitHub webook and/or b) faking data into the GitHub Status API. Or just c) have @dinosaure delete the https://ci.appveyor.com/project/dinosaure/checkseum-jx0qp/history project.

dinosaure commented 2 years ago

c) have @dinosaure delete the https://ci.appveyor.com/project/dinosaure/checkseum-jx0qp/history project.

It's done, thanks! I will try to do a release next week.