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.

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

implicit convertion to IntPtr from SafeHandle #12

Closed
NN--- opened this issue Sep 4, 2018 · 5 comments
Closed

implicit convertion to IntPtr from SafeHandle #12

NN--- opened this issue Sep 4, 2018 · 5 comments

Comments

@NN---
Copy link
Contributor

NN--- commented Sep 4, 2018

I don't think this is a good idea to have such implicit conversion.
It may lead to hard finding bugs.
The better option is to use SafeHandle instead of IntPtr.

public static implicit operator IntPtr(GenericSafeHandle h) => h.DangerousGetHandle();

@dahall
Copy link
Owner

dahall commented Sep 4, 2018

I hadn't considered that approach, but I like it. Let me look at what would break if I refactored everything that way.

@dahall
Copy link
Owner

dahall commented Sep 4, 2018

Turned out it wasn't so hard. Thanks for the suggestion. Code has been updated to make that operator explicit and I've updated affected functions.

@dahall dahall closed this as completed Sep 4, 2018
@NN---
Copy link
Contributor Author

NN--- commented Sep 4, 2018

Why not removing cast as .NET framework does.
The name DangerousGetHandle is what people are used to and for a good reason this is named so.

@dahall
Copy link
Owner

dahall commented Sep 4, 2018

Well, basically because I'm lazy and it would entail too many changes. Sorry.

dahall added a commit that referenced this issue Sep 4, 2018
… minimize unintended conversions (#12) and updated all affected functions, including fixing those affected by unintended consequences.
@NN---
Copy link
Contributor Author

NN--- commented Sep 4, 2018

If you remove implicit cast it is already a breaking change.
So better to remove the explicit too.
Perhaps you need to bump major version of the package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants