search

badcel / HidApi.Net

A modern cross platform C# binding for HIDAPI
https://badcel.github.io/HidApi.Net/
MIT License
36 stars 7 forks source link

Address CA1816: Call GC.SuppressFinalize correctly #60

Closed tmittet closed 12 months ago

tmittet commented 1 year ago

Not sure the potential issue actually applies to .Net Core. But this silences the warning.

badcel commented 1 year ago

Ah this is a complex topic. See discussion here.

According to current docs no finalizer is needed if a SafeHandle is used.

In the docs of the warning it reads like if the class gets sealed the warning should go away, too? At least it would make sense because if there is no finalizer and there is no derived class there will never be another finalizer thus making suppressing the finalizer call obsolete. See here.

So the question would be if the Device class is meant to be overriden: I'm not sure about that. As long as the SafeHandle member is private instead of protected a derived class can't really extend the functionality. It could only add new features add a higher level but those could be added if composition would be used over inheritance, too.

On the other hand I don't know what should be extended here, even if the SafeHandle member would be protected, except missing bindings. And missing bindings can be fixed with an update of this library. Everything else should be possible if the type is sealed, too.

So I tend to think let's seek the class. As this is an api break a major version bump would be needed.

What are your thoughts on this?

tmittet commented 1 year ago

I tend to make my disposable classes sealed, when I can, to avoid this warning. Inheritance is only useful in very rare cases and for this type I share your opinion, probably not useful at all :)

And then there's this: https://github.com/dotnet/csharpstandard/issues/291

tmittet commented 1 year ago

Thanks for the link to the discussion about Dispose and SafeHandles

badcel commented 1 year ago

Can you squash your two commits into one?

tmittet commented 12 months ago

Can you squash your two commits into one?

Done. I really like the ability to squash on merge in GitHub though, that way you get a tidy commit log and keep the history of the PR/feature branch

badcel commented 12 months ago

If I remember correctly it will be my commit which is authored by you if I squash it? As I'm not sure if a contributor likes this I prefer to merge it. In this way the timeline represents what happened even if it is not a straight timeline anymore.

Anyway, thank you for your contributions 🙏