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.
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
Unify nint and IntPtr #60913
Unify nint and IntPtr #60913
Changes from all commits
c3344a1
bb5a4bb
5bc9eb7
ba9723c
bf7bdc8
06df1b7
a7d8572
4a985bc
d455f4c
5961acd
42c3f9b
de4d98e
2f3afa9
679ca27
496d4be
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Where is this base implementation used? It looks like the only
AssemblySymbol
implementation that does not override this isRetargetingAssemblySymbol
. If so, consider making thisabstract
and adding an appropriate override for that case. #ResolvedThere 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.
There's also
MissingAssemblySymbol
.FWIW, I'm following the model from original DIM PR, trying to stick to an established pattern as much as possible (since we're going to have to do this regularly).
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.
Could this access and the one below go into an infinite loop when the current assembly is the CorLibrary? #Resolved
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 don't think so. If
CorLibrary
is available, then we'll callMetadataOrSourceAssemblySymbol.RuntimeSupportsNumericIntPtr
. If it's missing then we'll callMissingCorLibrarySymbol.RuntimeSupportsNumericIntPtr
.FWIW, I followed the same test hook pattern from DIM: #17927
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 confused by this. Are we using the current type as the containing type of the base 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.
You're right, base type would make more sense
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.
Should it be the containing type perhaps
null
, instead? In short, do we need to protect against a cycle when transforming a nested 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.
Chuck corrected both of us. It should be the containing type ;-)