move _WIN64 and _WIN32 defones to lib/Basic/Targets/OSTargets.h
move WIN32, WIN64 and MINGW64 to addMinGWDefines
remove unnecessary overrides
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
WIN32 and WIN64 are not real definitions they are typically defined by the system headers, _WIN32 and _WIN64 are the compiler definitions.
Almost.
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.
Comment Actions
... so in general I wouldn't mind doing a change like this, but I'd like to make it consistent on ARM, i386 and x86_64 at the same time in that case, instead of just changing aarch64.
Comment Actions
Thanks for the quick feedback :)
The easy one is to get rid of WIN64 because gcc doesn't even do that for mingw.
What are your thoughts here on WIN32, I would prefer to move it.
I remember a long time ago a lot of projects moved to _WIN32 when they discovered that it was not supposed to be defined.
I updated the diff to reflect removing WIN64
Comment Actions
... also WINNT was defined for X86 so I just added that to note it.
what do you suggest we do with that one.
Comment Actions
Yes it does, it behaves just the same as WIN32:
$ x86_64-w64-mingw32-gcc -E -dM - < /dev/null | grep WIN64 #define _WIN64 1 #define __WIN64 1 #define WIN64 1 #define __WIN64__ 1
What are your thoughts here on WIN32, I would prefer to move it.
I'm a little divided - either we remove both WIN32 and WIN64 from all mingw configurations, or we add the missing WIN32 for x86_64 mingw. Removing would be the strictly correct thing to do, but I'm sure it will break code that used to work before (even though it's wrong to rely on the unprefixed one).
I remember a long time ago a lot of projects moved to _WIN32 when they discovered that it was not supposed to be defined.
Yes, generally projects shouldn't check for the unprefixed defines, but unfortunately some do. I just recently ran into such an issue when building with the x86_64 configuration, where GCC would have had WIN32 defined but clang didn't.
Comment Actions
looking at different windef.h I see - specifically ReactOS
#ifndef WIN32 #define WIN32 #endif
I don't have MSVC off hand to check if it is there but I do remember MSVC projects used to pass /D "WIN32" so having them might make sense also.
I'm going to try include them and fold all the code for the different arch's for mingw in a moment
Comment Actions
LGTM
| test/Preprocessor/predefined-macros.c | ||
|---|---|---|
| 200 | In general, these kinds of -NOT tests don't work quite as one would want here - if the non-wanted #define _WIN64 would appear before the expected #define _WIN32, it would actually pass this test. Usually the -DAG kinds of checks can be used to avoid that issue. Not sure how well it mixes with -DAG and -DAG-NOT though. Can we assume that these defines appear in alphabetical order? In that case, we can be pretty sure to catch the issue if they're reordered slightly. This at least clearly documents what is the intended behaviour in any case. | |
In general, these kinds of -NOT tests don't work quite as one would want here - if the non-wanted #define _WIN64 would appear before the expected #define _WIN32, it would actually pass this test. Usually the -DAG kinds of checks can be used to avoid that issue. Not sure how well it mixes with -DAG and -DAG-NOT though.
Can we assume that these defines appear in alphabetical order? In that case, we can be pretty sure to catch the issue if they're reordered slightly. This at least clearly documents what is the intended behaviour in any case.