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

Update SafeHandle types to have public parameterless constructors and add InitHandle API #47189

Merged
5 commits merged into from
Jan 20, 2021
Merged

Update SafeHandle types to have public parameterless constructors and add InitHandle API #47189

5 commits merged into from
Jan 20, 2021

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jan 19, 2021 •
edited by AaronRobinsonMSFT
Loading

Implements the API changes described in #46830.

Make all non-abstract SafeHandle types have public parameterless constructors. Add the InitHandle API that initializes the underlying handle of the SafeHandle object.

Contributes to #46830

See #45633 for linker context.

@jkoritzinsky jkoritzinsky added this to the 6.0.0 milestone Jan 19, 2021
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

AaronRobinsonMSFT
AaronRobinsonMSFT approved these changes Jan 19, 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. I would also add a test for the new Marshal API.

@AaronRobinsonMSFT
Copy link
Member

/cc @eerhardt

@eerhardt
Copy link
Member

Can you delete the following files along with this, as they are no longer necessary?

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Sockets/src/ILLink/ILLinkTrim_LibraryBuild.xml
https://github.com/dotnet/runtime/blob/master/src/libraries/System.IO.MemoryMappedFiles/src/ILLink/ILLinkTrim_LibraryBuild.xml

@jkoritzinsky
Update public SafeHandle types to have public parameterless construct…
6efd06b 
…ors.

Remove now-unneeded ILLink trim files.

Make public abstract SafeHandle constructors protected.

Remove comment on SafeWaitHandle
@jkoritzinsky jkoritzinsky force-pushed the safehandle-apis branch 2 times, most recently from 790c886 to cdb9780 Compare January 19, 2021 23:37
@jkoritzinsky
Make SafeHandle constructors public for internal types.
923f6f5 
Make internal abstract SafeHandle default constructors protected. Make non-abstract internal SafeHandles sealed when possible.

More internal SafeHandle types.
@jkoritzinsky
Add new Marshal.InitHandle API.
7b99f90 
Fix SetHandle accessibility

Fix test.

Use Marshal.InitHandle.
@jkoritzinsky
Copy link
Member Author

@eerhardt @stephentoub any more changes needed?

eerhardt
eerhardt approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Just missed a couple sealed classes.

src/installer/managed/Microsoft.NET.HostModel/ResourceUpdater.cs Outdated Show resolved Hide resolved
...braries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Keychain.cs Show resolved Hide resolved
...braries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Keychain.cs Show resolved Hide resolved
@ghost
Copy link

ghost commented Jan 20, 2021

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 3cbdf4c into dotnet:master Jan 20, 2021
@jkoritzinsky jkoritzinsky deleted the safehandle-apis branch January 21, 2021 19:29
@ghost ghost locked as resolved and limited conversation to collaborators Feb 20, 2021
This pull request was closed.
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 approved these changes

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

@elinor-fung elinor-fung Awaiting requested review from elinor-fung

@jkotas jkotas Awaiting requested review from jkotas

Assignees
No one assigned
Labels
area-System.Runtime.InteropServices new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jkoritzinsky @AaronRobinsonMSFT @eerhardt @stephentoub