-
-
Notifications
You must be signed in to change notification settings - Fork 228
Add NativeOverlapped overloads #495
Add NativeOverlapped overloads #495
Conversation
Hmm, having overloads with both |
This is a binary breaking change. AFAIK we haven't introduced any lately, so I hesitate to take one now unless we're prepared to do a bunch of others as well to minimize the number of releases with breaking changes over time. #288 tracks many ideas for breaking changes. |
@AArnott Alternatively, we can add this as a new overload in a |
What if we just add conversion operators to our struct so it can be cast to the other one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've heard from the .NET interop folks that each p/invoke function has a fixed cost and they were very happy to hear that we had just one per Win32 function and all our overloads were just thunks into that same one. Rather than creating new p/invokes for the same function, can you add methods to the helper file that simply re-cast the pointer from NativeOverlapped*
to OVERLAPPED*
? That way we have more performant, less memory hungry code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect. But we could trim down the xml doc comment weight of it significantly. See my tip.
In line with #419, and also based on my own experience, add overloads which use the
System.Threading.NativeOverlapped
struct instead ofKernel32+OVERLAPPED
. .NET has built-in support for overlapped I/O, so let's use that instead of rolling our own.In line with the guidance in #487, obsolete these types/overloads instead of removing them.
Closes #419