Skip to content
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

Use C# compiler provided nint/nuint #36159

Merged
merged 5 commits into from
May 9, 2020
Merged

Use C# compiler provided nint/nuint #36159

merged 5 commits into from
May 9, 2020

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented May 9, 2020

No description provided.

@jkotas jkotas mentioned this pull request May 9, 2020
Closed
@GrabYourPitchforks
Copy link
Member

Is it safe to use nint and nuint within the IntPtr and UIntPtr classes, respectively? I wonder if this could lead to some weird recursive or unstable code paths.

@jkotas
Copy link
Member Author

jkotas commented May 9, 2020

some weird recursive or unstable code paths.

Yes, the tests caught it too

@GrabYourPitchforks
Copy link
Member

Just to confirm, it should not be a breaking change if in the future we decide to change APIs like Marshal.AllocHGlobal and Unsafe.Add's parameters from IntPtr to nint, correct? In my limited testing implicit conversions seem to work just fine.

@EgorBo
Copy link
Member

EgorBo commented May 9, 2020

Does it make System.Private.CoreLib AnyCPU-friendly?

@tannergooding
Copy link
Member

Does it make System.Private.CoreLib AnyCPU-friendly?

Corelib still has other ifdefs and compilation switches for endianness and ARM/ARM64 vs x86/x64 intrinsics at the very least

@john-h-k
Copy link
Contributor

john-h-k commented May 9, 2020

Corelib still has other ifdefs and compilation switches for endianness and ARM/ARM64 vs x86/x64 intrinsics at the very least

It also uses it for some of the memory copy/set methods and the IntPtr/UIntPtr implementations

@jkotas
Copy link
Member Author

jkotas commented May 9, 2020 •
edited
Loading

Just to confirm, it should not be a breaking change if in the future we decide to change API

It would not be a binary breaking change. I suspect that it may be a source breaking change in some rare cases.

@jkotas jkotas merged commit 6b5850f into dotnet:master May 9, 2020
@jkotas jkotas mentioned this pull request May 9, 2020
Closed
eerhardt
eerhardt reviewed May 9, 2020
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Utf8String.cs
@@ -133,7 +119,7 @@ public int CompareTo(Utf8String? other, StringComparison comparison)
#if SYSTEM_PRIVATE_CORELIB
return ref Unsafe.AddByteOffset(ref DangerousGetMutableReference(), index);
#else
return ref Unsafe.AddByteOffset(ref DangerousGetMutableReference(), (IntPtr)index);
return ref Unsafe.AddByteOffset(ref DangerousGetMutableReference(), (nint)index);
Copy link
Member

Choose a reason for hiding this comment

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

I think the whole #if can be removed here now. I can do that in a follow up.

@jaredpar jaredpar mentioned this pull request May 11, 2020
Closed
@GrabYourPitchforks GrabYourPitchforks mentioned this pull request May 11, 2020
Merged
@eerhardt
Copy link
Member

eerhardt commented May 11, 2020 •
edited
Loading

Just to confirm, it should not be a breaking change if in the future we decide to change API

It would not be a binary breaking change. I suspect that it may be a source breaking change in some rare cases.

Would changing a return value or out parameter from IntPtr to nint be a breaking change since some methods on IntPtr are not available on nint. e.g. ToInt32()?

@GrabYourPitchforks
Copy link
Member

Seems like a good example of the possible source breaking changes Jan alluded to. But you'd have to retarget + recompile your app to see the breaks. The ToInt32() method still exists, so existing compiled DLLs brought forward to the new runtime shouldn't be affected.

@john-h-k
Copy link
Contributor

john-h-k commented May 11, 2020 •
edited
Loading

Would changing a return value or out parameter from IntPtr to nint be a breaking change since some methods on IntPtr are not available on nint. e.g. ToInt32()?

wouldn't changing an out parameter be a breaking change as out params must be invariant? or does the compiler special case this

@jkotas jkotas deleted the native-int branch May 16, 2020 17:11
@tannergooding tannergooding mentioned this pull request Dec 4, 2020
Open
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cston cston cston left review comments

@tannergooding tannergooding tannergooding left review comments

@eerhardt eerhardt eerhardt left review comments

@stephentoub stephentoub stephentoub approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@EgorBo EgorBo Awaiting requested review from EgorBo

@marek-safar marek-safar Awaiting requested review from marek-safar

@steveisok steveisok Awaiting requested review from steveisok

@vargaz vargaz Awaiting requested review from vargaz

Assignees
No one assigned
Labels
area-Meta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jkotas @GrabYourPitchforks @EgorBo @tannergooding @john-h-k @eerhardt @stephentoub @cston @Dotnet-GitSync-Bot