Skip to content

Managed Renames for consistency #28819

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Feb 5, 2020
Merged

Managed Renames for consistency #28819

merged 8 commits into from
Feb 5, 2020

Conversation

sdmaclea
Copy link
Contributor

@sdmaclea sdmaclea commented Feb 1, 2020 •
edited
Loading

Rename coreclr managed conditional compilation macros to match CoreCLR native concepts

BIT64                   -> TARGET_64BIT
BIT32                   -> TARGET_32BIT
FEATURE_PAL             -> TARGET_UNIX
PLATFORM_UNIX           -> TARGET_UNIX
PLATFORM_WINDOWS        -> TARGET_WINDOWS
PLATFORM_OSX            -> TARGET_OSX

Remove obsolete logic and obsolete define: FeaturePal

@sdmaclea sdmaclea requested a review from jkotas February 1, 2020 01:07
@sdmaclea sdmaclea self-assigned this Feb 1, 2020
@sdmaclea sdmaclea added this to the 5.0 milestone Feb 1, 2020
@jkotas
Copy link
Member

jkotas commented Feb 1, 2020

PLATFORM_WINDOWS

These are used under src/libraries as well. It needs to be renamed there as well.

Note some of the managed files are shared between src/libraries and src/coreclr. I think the PR is not going to pass the tests without also fixing src/libraries.

@jkotas
Copy link
Member

jkotas commented Feb 1, 2020

And under src/mono too: https://github.com/dotnet/runtime/blob/master/src/mono/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj#L82

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Feb 3, 2020

@jkotas, I think I have handled all the managed code.

I am not sure why the mono legs are failing. The failure look unrelated. They actually look related to the unchanged check failures Beta reproted in the diff page. Are the mono legs expected to pass?

jkotas
jkotas reviewed Feb 3, 2020
View reviewed changes
jkotas
jkotas reviewed Feb 3, 2020
View reviewed changes
@jkotas
Copy link
Member

jkotas commented Feb 3, 2020

Are the mono legs expected to pass?

Yes, there are expected to pass. One of my comments flagged the place that is causing the build break you are seeing.

@monojenkins monojenkins mentioned this pull request Feb 3, 2020
Closed
@lambdageek
Copy link
Member

Please undo the (whitespace-only) changes to src/mono/mono/arch/arm/arm-wmmx.h and src/mono/mono/mini/genmdesc.py

@sdmaclea
Managed Renames for consistency
192a0b5 
Rename managed conditional compilation macros to match CoreCLR native concepts

    BIT64                   -> TARGET_64BIT
    BIT32                   -> TARGET_32BIT
    FEATURE_PAL             -> TARGET_UNIX
    PLATFORM_UNIX           -> TARGET_UNIX
    PLATFORM_WINDOWS        -> TARGET_WINDOWS
    PLATFORM_OSX            -> TARGET_OSX

Remove obsolete logic and obsolete defines: FeaturePal
@sdmaclea
Copy link
Contributor Author

sdmaclea commented Feb 3, 2020

Please undo the (whitespace-only) changes to src/mono/mono/arch/arm/arm-wmmx.h and src/mono/mono/mini/genmdesc.py

@lambdageek Sure. However I would apreciate it if the mono team fixed the broken whitespace issue. git is making me fight these incorrectly committed files. #2222

@sdmaclea sdmaclea requested a review from jkotas February 4, 2020 16:03
This was referenced Feb 4, 2020
Open
Merged
jkotas
jkotas reviewed Feb 4, 2020
View reviewed changes
jkotas
jkotas approved these changes Feb 4, 2020
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise. Thanks!

@sdmaclea sdmaclea merged commit 02d6419 into dotnet:master Feb 5, 2020
@sdmaclea sdmaclea deleted the TARGET_UNIX branch February 5, 2020 00:54
@jaredpar jaredpar mentioned this pull request Feb 5, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marek-safar marek-safar marek-safar left review comments

@jkotas jkotas jkotas approved these changes

Assignees

@sdmaclea sdmaclea

Labels
area-Diagnostics-coreclr
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sdmaclea @jkotas @lambdageek @marek-safar