-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Updating usages of IntPtr
/UIntPtr
to be the language keyword nint
/nuint
now that they are equivalent
#70297
Closed
Closed
Updating usages of IntPtr
/UIntPtr
to be the language keyword nint
/nuint
now that they are equivalent
#70297
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
8279ed2
Updating usages of `UIntPtr` to be the language keyword `nuint` now t…
tannergooding 0253e39
Updating usages of `IntPtr` to be the language keyword `nint` now tha…
tannergooding 7f7b4e3
Fixing a couple of bugs
tannergooding 1bd3a5a
Fixing a Unix specific build failure
tannergooding File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
This and many other places are pointers.
nint
makes them look odd. We should be changing these tovoid*
instead.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'm happy to do that, but just noting its going to make this a much more in depth change.
Also noting that this isn't critical for .NET 7, its mostly "stylistic" cleanup outside the places where
IntPtr.Zero
and similar were swapped to something more efficient.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.
That's sounds fine to me. It can be done in chunks to make it reviewable.
I would switch to
void*
or other appropriate pointer type where possible, and keep usingIntPtr
for pointers where it is not possible to switch. Switch to nint/nuint only in places where the value is not pointer.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.
This is going to conflict, longer term, with editorconfig options and what IntelliSense/docs display. As of C# 11 (when targeting .NET 7 or later),
IntPtr/nint
andUIntPtr/nuint
are true aliases of eachother, in the same way asInt32/int
andUInt32/uint
. They are identical in IL (not even aNativeInteger
attribute any longer) and all the same style rules and support will exist/apply.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.
In my opinion it is about intent. Yes, in IL they are identical aliases for each other. But the important thing here is for the code reader, not the compiler or tools.
What is the intent of this variable? Is it "just a number" like a size or a counter? Or is it a pointer?
I find it super confusing to use
nint
for "pointer". As a reader, you need to inspect further to figure out what the intention of the variable is than just looking at its type.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.
Yes. But IntelliSense does and IntelliSense is going to display
nint
everywhere, just as it displaysint
everywhere.IntelliSense "could" respect https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0049, but that would be
IntPtr
andInt32
everywhere withnint
andint
nowhere.https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md.
Now that
IntPtr
andnint
are true aliases of eachother,nint
is a keyword and is per our rules and the general styling/editorconfig rules, preferred.Users are going to have to update their incorrect expectations here and change. This is and has always been the case for many languages outside of C#.
We can improve some things on our own end by utilizing pointers instead, where feasible. But trying to continue perpetuating the leaky abstraction that
IntPtr
is somehow different fromnint
is flawed and not going to end up in a positive area.This was already discussed in depth by API review and C# LDM with consideration for how other languages do things, how APIs are exposed, and what our official guidance for exposing such APIs is.
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.
Whatever precedence we set in our own code is one that, for better or worse, is going to be used as a representative example for other codebases.
The handful of APIs that fall into the weird category of "this should have been
void*
but the API review committee at the time had an aversion to unsafe code" will be exposed to the end user asnint
and that is how everyone is going to see that API in VS, regardless of what we use in source.Because of that, just being consistent with our own existing rules and guidelines should be fine and will help ensure that source link vs decompilation match up.
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.
Agree. I believe that blank find&replace of IntPtr with nint is a bad precedence to set. We should set the precedence by converting IntPtr pointer values to use proper unmanaged pointer values where possible.
As the language evolves, the coding style has to evolve as well. I am proposing to add a new rule to the repo style rule:
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.
Right, I agree with that. The remaining bit is whether
AllocHGlobal
is declared in source asIntPtr AllocHGlobal(nint cb)
or asnint AllocHGlobal(nint cb)
.IntelliSense, tooling, and other bits are all going to show and default to the latter for .NET 7+ and so I think we should be consistent there. The proposed rule you gave should be that we convert the returned
nint
tovoid*
at all usage sites, since we can't actually change the API signature to returnvoid*
.It might be beneficial to have an analyzer to help flag these edge cases, but that's not a strict need.
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 do not have a strong opinion on that. It is going to need a comment that explains that it is actually a pointer. It can be also declared as
nint /* IntPtr */ AllocHGlobal(nint cb)
ornint /* void* */ AllocHGlobal(nint cb)
to get the point across.