Move System.Drawing.Common to SafeHandles et. al.
#63085
Conversation
And fix some P/Invoke definitions.
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-drawing Issue DetailsThe interop code of For this reason and to also improve the library's stability ( I have created a (currently internal) subclass of I will also fix other minor things I might encounter.
|
|
Looks like you need to add public parameterless constructors to each of the SafeHandle-derived types you're introducing in this PR. |
|
Thanks @jkoritzinsky, I had noticed it as well. I opened #63109 to suggest an analyzer that detects cases like that. |
teo-tsirpanis
force-pushed
the
sdc-safehandle
branch
from
5348506
to
56cba2c
Compare
|
The tests are failing because they are trying to access disposed handles. It used to fail with There will be a breaking change, but I think it is for the better; the overall improvements brought by the |
src/libraries/System.Drawing.Common/src/System/Drawing/GdiplusNative.Unix.cs
Outdated
Show resolved
Hide resolved
|
What about your new HandleRef's in this code? Do you plan to make those SafeHandles as well? |
|
Yes @AraHaan, I was consolidating some interop code that had |
It's sealed and no longer has a finalizer. It does not need the full-blown Dispose pattern.
|
Is there plans to also backport these changes to 6.0 in a servicing release as well? |
|
Almost certainly not @AraHaan, this is quite a significant change, and the different exceptions thrown when accessing disposed objects will cause breaks. |
Accessing a disposed handle now throws ObjectDisposedException instead of ArgumentException.
And rename some parameter names in the manually marshalled P/Invoke definitions.
teo-tsirpanis
force-pushed
the
sdc-safehandle
branch
from
a0ba9ba
to
6b60b9c
Compare
| @@ -78,7 +78,7 @@ public int NextSubpath(out int startIndex, out int endIndex, out bool isClosed) | |||
| public int NextSubpath(GraphicsPath path, out bool isClosed) | |||
There was a problem hiding this comment.
Should it have been a GraphicsPath? here and in a similar case below? Both APIs work when null is passed.
| @@ -13,7 +13,7 @@ public sealed class GraphicsPathIterator : MarshalByRefObject, IDisposable | |||
| public GraphicsPathIterator(GraphicsPath? path) | |||
| { | |||
| IntPtr nativeIter = IntPtr.Zero; | |||
There was a problem hiding this comment.
Should this be SafeGraphicsPathIteratorHandle?
There was a problem hiding this comment.
Didn't port it yet. The PR is still in progress and not yet ready for review.
|
I will pause working on this until the custom |
And shorten SafeMatrixHandle.HasEqualHandle.
ghost
closed this
|
Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it. |
|
Fair enough, I will open a new one once it is ready. |
ghost
locked as resolved and limited conversation to collaborators
The interop code of
System.Drawing.Commonheavily uses the very oldHandleReftype which predates the widely used and saferSafeHandles that .NET Framework 2.0 brought. Its use of this legacy type has become an issue since the P/Invoke generator doesn't support marshallingHandleRefs.For this reason and to also improve the library's stability (
SafeHandlesare strongly-typed and harder to misuse and I have seenIntPtrs in P/Invoke signatures that should have beenHandleRefs), this PR will replace all usages ofHandleRefwithSafeHandles.I have created a (currently internal) subclass of
SafeHandlecalledSafeGdiPlusHandle, which will be inherited by one class per GDI+ handle type (SafeBrushHandle,SafePenHandleetc). dotnet/winforms#8833 will need to be reconsidered and perhaps superseded by a new proposal that makes the safe handles public.I will also fix other minor things I might encounter.