-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename cross compilation related defines #2256
Conversation
src/coreclr/clr.featuredefines.props
Outdated
@@ -60,7 +60,7 @@ | |||
<DefineConstants Condition="'$(FeatureCominteropWinRTManagedActivation)' == 'true'">$(DefineConstants);FEATURE_COMINTEROP_WINRT_MANAGED_ACTIVATION</DefineConstants> | |||
<DefineConstants Condition="'$(FeatureManagedEtw)' == 'true'">$(DefineConstants);FEATURE_MANAGED_ETW</DefineConstants> | |||
<DefineConstants Condition="'$(FeatureManagedEtwChannels)' == 'true'">$(DefineConstants);FEATURE_MANAGED_ETW_CHANNELS</DefineConstants> | |||
<DefineConstants Condition="'$(FeaturePal)' == 'true'">$(DefineConstants);FEATURE_PAL</DefineConstants> | |||
<DefineConstants Condition="'$(FeaturePal)' == 'true'">$(DefineConstants);TARGET_UNIX</DefineConstants> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be nice to delete FeaturePal
as well (in future PR).
5fbd3db
to
6095894
Compare
We have choose |
It was currently set based on the host architecture. It looks wrong in most places, but I wanted to make this mostly a non-functional change. Given the JIT/crossgen supports cross bitness builds, I didn't want to mess with this here. I also switched the PLATFORM_UNIX to map to HOST_UNIX on the similar rationale. |
Ok, hope most them are going to be flipped to be |
I certainly want to minimize |
I had originally been doing the bulk edit replaces in my editor. Turns out my editor was skipping some files for some reason.... Anyways I decided it was less error prone to script this. So I wrote a bash/git/sed script to replace a single editor within a git subtree. Then I wrote a script for all the bulk edits. I kept the commits more separate to ease review burden. The bulk search replace are separate commits. The manual edits are separate commits.
|
b61329b
to
75157b4
Compare
This patch was a machine generated search and replace of identifiers. The replacement was done in the order listed for the path specified. A regex was used to find the identifiers. s/(^|[^A-CE-Za-z0-9_])${find}($|[^A-Za-z0-9_]) The 'D' character was specifically allowed a s a prefix to catch add_definitions(-D${find}). The patch also reverts all managed code changes after replacement. _ARM_ -> HOST_ARM in src/coreclr _ARM64_ -> HOST_ARM64 in src/coreclr _AMD64_ -> HOST_AMD64 in src/coreclr _X86_ -> HOST_X86 in src/coreclr _HOST_UNIX_ -> HOST_UNIX in src/coreclr _HOST_AMD64_ -> HOST_AMD64 in src/coreclr _HOST_ARM64_ -> HOST_ARM64 in src/coreclr _HOST_ARM_ -> HOST_ARM in src/coreclr _HOST_X86_ -> HOST_X86 in src/coreclr DBG_TARGET_AMD64 -> TARGET_AMD64 in src/coreclr DBG_TARGET_ARM64 -> TARGET_ARM64 in src/coreclr DBG_TARGET_ARM -> TARGET_ARM in src/coreclr DBG_TARGET_X86 -> TARGET_X86 in src/coreclr _TARGET_AMD64_ -> TARGET_AMD64 in src/coreclr _TARGET_ARM_ -> TARGET_ARM in src/coreclr _TARGET_ARM64_ -> TARGET_ARM64 in src/coreclr _TARGET_X86_ -> TARGET_X86 in src/coreclr _TARGET_ARMARCH_ -> TARGET_ARMARCH in src/coreclr _TARGET_XARCH_ -> TARGET_XARCH in src/coreclr _HOST_64BIT_ -> HOST_64BIT in src/coreclr BIT64 -> HOST_64BIT in src/coreclr _TARGET_64BIT_ -> TARGET_64BIT in src/coreclr DBG_TARGET_64BIT -> TARGET_64BIT in src/coreclr HOST_IS_WINDOWS_OS -> HOST_WINDOWS in src/coreclr PLATFORM_UNIX -> HOST_UNIX in src/coreclr/*.cmake PLATFORM_WINDOWS -> HOST_WINDOWS in src/coreclr/*.cmake _TARGET_MAC64 -> TARGET_OSX in src/coreclr FEATURE_PAL -> TARGET_UNIX in src/coreclr _TARGET_UNIX_ -> TARGET_UNIX in src/coreclr _TARGET_WINDOWS_ -> TARGET_WINDOWS in src/coreclr PLATFORM_UNIX -> TARGET_UNIX in src/coreclr PLATFORM_WINDOWS -> TARGET_WINDOWS in src/coreclr
4735c94
to
08e3951
Compare
Remove unused defines Remove BIT32 Remove DBG_TARGET_AMD64_UNIX Remove DBG_TARGET_ARM64_UNIX Remove DBG_TARGET_ARM_UNIX Remove DBG_TARGET_32BIT Fixes for HOST_<arch> rename Move TARGET_<Arch> and TARGET_<bit> Move from clrdefinitions.cmake to configurecompiler.cmake so it is used globally. More jit.h
fwiw, this change deserved to be (and still deserves to be) much more widely before being merged. Anyone with in-progress changes that added architecture-specific code (for example), then rebased, might not get the behavior they expect because previously-working @dotnet/jit-contrib fyi: many global Also, can we change |
There is discussion about it here: #31659 (comment) . We have OSX name in the number of places today that we cannot change (e.g. https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.osplatform.osx?view=netframework-4.8). If we want to change it to something, I would vote for DARWIN. DARWIN should be immune to marketing whims. |
Can we make the old names cause errors for some interim period, so it's more obvious they won't work anymore? Otherwise stuff may just silently get dropped. |
I filed #31732 to clean up some inconsistencies in the JIT |
@dotnet/jit-contrib My apologies for not socializing this earlier. I had been concerned that my PR would shear with respect to master. That somehow I would have missed a rename.... I checked immediately after I merged so that I could prevent an outage. I didn't really anticipate the possibility that other PRs might go in after w/o having merge conflicts with this one.... @AndyAyersMS I am not sure how to make use of an undefined macro cause a build break. Something like #define _TARGET_ARM_ static_assert(BadDefine != BadDefine, "_TARGET_ARM_ macro is obsolete. Use TARGET_ARM") Might work in some circumstances, but not in all. I don't think it would work in the most common cases. Particularly: #ifdef _TARGET_ARM_ #if defined(_TARGET_ARM_) |
Will |
For non-MSVC compilers, that #define WARN(msg) WARN2(GCC warning msg)
#define WARN2(s) _Pragma(#s)
#define DEPRECATE_MACRO WARN("Macro is deprecated") And then use it like so: #define _TARGET_ARM_ DEPRECATE_MACRO I don't know, however, if there's something similar for C++ or even MSVC, though. |
While this mechanism works with clang, it doesn't help our purpose. The problem is that it only warns for |
A future PR will rename some of the TARGET_* to HOST_* and vice-versa as needed. This mostly a brute force search and replace.