This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Remove mentions of Rotor from codebase #20298
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It seems a bit odd to have the constructor parsing and then use a dummy method (MakeRotorHappy) to make it look more normal.
It is neither referenced nor exported.
This file is only built for Windows.
This is the only file the defines these macros, so there is no need to undef them first.
I can't find any evidence that this file is actually used.
FEATURE_PROFAPI_EVENT_LOGGING is always defined when PROFILING_SUPPORTED is defined. And the entire contents of profilinghelper.cpp is surrounded with "ifdef PROFILING_SUPPORTED". So all sections in "ifndef FEATURE_PROFAPI_EVENT_LOGGING" are dead. Furthermore, in coreclr this does not use the eventlog, so the macro name is misleading.
This entire function is surrounded with "ifndef FEATURE_PAL".
This does not appear to cause any compile problems, so nobody was using safemath.h without _ASSERTE defined. Also S_SIZE_T_WP64BUG is not used anywhere.
I don't know why these check to see if the macro is undefined immediately after defining them. Also the comment appears to reference some unions that are no longer in this file.
The comment talks about the C# compiler using this, however I cannot see a way for the C# compiler to get an instance of this. It is only used internally by AssemblyBuilder and not exposed otherwise.
|
Some of the changes might be more risky than others, so I have split this PR into many commits. That way if something is problematic, it can be easily reverted. |
|
Build break:
|
On Windows sometimes that this file is included without _ASSERTE being defined. As the existing comment suggests, it appears that SOS explicitly does not want _ASSERTE to do anything.
|
Interestingly, I did not have any trouble with building this on Linux. So that might mean linux builds have a few more asserts than Windows builds. Perhaps this is something worth investigating in the future? I reverted my changes in safemath.h for now. |
jkotas
reviewed
Oct 8, 2018
jkotas
reviewed
Oct 8, 2018
jkotas
reviewed
Oct 8, 2018
|
Looks good modulo a few nits. Thank you! |
|
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please |
A-And
pushed a commit
to A-And/coreclr
that referenced
this pull request
Nov 20, 2018
* Moving parsing from TypeNameParser ctor to a separate method. It seems a bit odd to have the constructor parsing and then use a dummy method (MakeRotorHappy) to make it look more normal. * Remove CorMarkThreadInThreadPool. It is neither referenced nor exported. * Remove reference to rotor from securitywrapper.h * Remove reference to rotor from Strike/vm.cpp. This file is only built for Windows. * Remove reference to rotor from debugreturn.h This is the only file the defines these macros, so there is no need to undef them first. * Remove unused code refering to rotor from PAL. * Remove references to Rotor from PAL. * Remove references to deleted tests from DisabledTests.txt I can't find any evidence that this file is actually used. * Remove unneeded casts. * Remove dead and misleading code from profilinghelper.cpp. FEATURE_PROFAPI_EVENT_LOGGING is always defined when PROFILING_SUPPORTED is defined. And the entire contents of profilinghelper.cpp is surrounded with "ifdef PROFILING_SUPPORTED". So all sections in "ifndef FEATURE_PROFAPI_EVENT_LOGGING" are dead. Furthermore, in coreclr this does not use the eventlog, so the macro name is misleading. * Remove dead code in excep.cpp. This entire function is surrounded with "ifndef FEATURE_PAL". * Remove refererences to rotor from safemath.h This does not appear to cause any compile problems, so nobody was using safemath.h without _ASSERTE defined. Also S_SIZE_T_WP64BUG is not used anywhere. * Remove dead code from palclr.h. I don't know why these check to see if the macro is undefined immediately after defining them. Also the comment appears to reference some unions that are no longer in this file. * Expose ISymUnmanagedWriter2 from SymWriter as required by COM. The comment talks about the C# compiler using this, however I cannot see a way for the C# compiler to get an instance of this. It is only used internally by AssemblyBuilder and not exposed otherwise. * Restore check for _ASSERTE in safemath.h. On Windows sometimes that this file is included without _ASSERTE being defined. As the existing comment suggests, it appears that SOS explicitly does not want _ASSERTE to do anything.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Many references to Rotor in this codebase are confusing, as they tend to contrast Rotor's behavior with .NET Framework (neither of which matches CoreCLR) or reference compatibility problems with 15 year old compilers.
This series of commits should remove all mentions rotor from the codebase, except from the many non-compiling PAL tests. Many of them reflect a time when the PAL was built as a shared library, so once they are cleaned up they will be purged of the reference to
librotor_pal.Fixes #9872