Skip to content

Use C# compiler provided nint/nuint#36159

Merged
jkotas merged 5 commits intodotnet:masterfrom
jkotas:native-int
May 9, 2020
Merged

Use C# compiler provided nint/nuint#36159
jkotas merged 5 commits intodotnet:masterfrom
jkotas:native-int

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

GrabYourPitchforks commented May 9, 2020

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

GrabYourPitchforks commented May 9, 2020

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

tannergooding commented May 9, 2020

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
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

@eerhardt eerhardt May 9, 2020

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

GrabYourPitchforks commented May 11, 2020

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

@stephentoub stephentoub stephentoub approved these changes

@eerhardt eerhardt eerhardt left review comments

@tannergooding tannergooding tannergooding left review comments

@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

+1 more reviewer

@cston cston cston left review comments

Reviewers whose approvals may not affect merge requirements

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