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
Parse CallConvSwift in CoreCLR/NativeAOT #96707
Parse CallConvSwift in CoreCLR/NativeAOT #96707
Changes from all commits
a7e3ee4
8027343
e056d69
d667148
db1abd0
9b642c0
ebe34e0
e873f64
de1f23a
2ac4812
fc5c9ff
0e9934b
82764a1
b115352
4bea8a5
1b8eb2c
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.
This method doesn't appear used. Is that intentional?
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 removed the usage of it during a refactor. I think it's still useful to have, but we can remove it and re-add later when we have a use case.
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.
Nit: Might want to either do
delegateType.GetTypeDefinition() is EcmaType ecmaDelegate
or assert the type is not generic so that we just don't do bad codegen if we ever allow interop with generic delegates.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.
The
callingConvention
parameter is not used, is it intentional?Why do we need a new overload though? I would expect the existing
IsMarshallingRequired
to get an extracallingConvention
argument if answer to this now depends on the calling convention as well (or why can we still decide it without the calling convention in the other overload?).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 the other overload, we can extract the calling convention from the MethodSignature. This overload is for UnmanagedCallersOnly methods where the callconv is in the attribute, not the managed signature.
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.
The parameter is still unused, is that intentional?
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, it's intentional that it's unused.