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

Fix inconsistency in p/invoke arguments introduced when switching some to be blittable #61071

Merged
merged 1 commit into from
Nov 10, 2021
Merged

Fix inconsistency in p/invoke arguments introduced when switching some to be blittable #61071

merged 1 commit into from
Nov 10, 2021

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung added this to the 7.0.0 milestone Nov 1, 2021
jkoritzinsky
jkoritzinsky reviewed Nov 1, 2021
View reviewed changes
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

I think @stephentoub wanted us to go in the other (non-blittable byrefs) direction, not making everything pointers.

AaronRobinsonMSFT
AaronRobinsonMSFT approved these changes Nov 1, 2021
View reviewed changes
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

LGTM.

src/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs Outdated Show resolved Hide resolved
@elinor-fung
Copy link
Member Author

I had interpreted #59658 (comment) (and therefore #59658 (comment)) as making them all pointers, since in this case, unlike some of the bool<->int ones we had, it doesn't make them much less usable.

@jkoritzinsky
Copy link
Member

I remember during our internal discussions that there was a dislike about losing the conceptual ideas that the out annotation brings (that the parameter doesn't need to be initialized before being passed to native code) that we may have forgotten to write down.

@elinor-fung
Copy link
Member Author

Ah, I do recall discussion about out - for whatever reason, that I lumped that with the bool in my mind. Thoughts, @stephentoub?

deeprobin
deeprobin approved these changes Nov 3, 2021
View reviewed changes
Copy link
Contributor

@deeprobin deeprobin left a comment

Choose a reason for hiding this comment

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

LGTM

@elinor-fung
Copy link
Member Author

elinor-fung commented Nov 9, 2021 •
edited
Loading

Went back and looked at some notes and what they were originally - switched back to out/ref.

@elinor-fung elinor-fung merged commit 213020c into dotnet:main Nov 10, 2021
@elinor-fung elinor-fung deleted the pinvokeInconsistency branch November 10, 2021 21:18
This was referenced Nov 29, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkoritzinsky jkoritzinsky jkoritzinsky approved these changes

@deeprobin deeprobin deeprobin approved these changes

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

Assignees
No one assigned
Labels
area-System.Runtime.InteropServices
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

[DllImportGenerator] Fix inconsistencies in signature of updated p/invokes
4 participants
@elinor-fung @jkoritzinsky @deeprobin @AaronRobinsonMSFT