Skip to content
This repository was archived by the owner on Jul 26, 2023. It is now read-only.

Add NativeOverlapped overloads #495

Merged
merged 3 commits into from
Jul 19, 2020
Merged

Add NativeOverlapped overloads #495

merged 3 commits into from
Jul 19, 2020

Conversation

qmfrederik
Copy link
Contributor

In line with #419, and also based on my own experience, add overloads which use the System.Threading.NativeOverlapped struct instead of Kernel32+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

@qmfrederik
Copy link
Contributor Author

Hmm, having overloads with both OVERLAPPED* and NativeOverlapped* structs generates conflicts (the code generator would generate two identical overloads with IntPtr parameters), so I removed OVERLAPPED* for now. Happy to revisit if needed.

@AArnott
Copy link
Collaborator

AArnott commented Jul 8, 2020

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.
We could perhaps start a branch with changes like these and merge them all at once, although I'm concerned about merge conflicts too, so I prefer to keep a list of ideas for changes and then we can work on them all at once and merge more quickly.

@qmfrederik
Copy link
Contributor Author

@AArnott Alternatively, we can add this as a new overload in a .Helper.cs file; but at the moment the code generators would generate an IntPtr-based overload, which would cause conflicts.

@AArnott
Copy link
Collaborator

AArnott commented Jul 8, 2020

What if we just add conversion operators to our struct so it can be cast to the other one?

Copy link
Collaborator

@AArnott AArnott left a 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.

Copy link
Collaborator

@AArnott AArnott left a 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.

@AArnott AArnott merged commit b48b2cb into dotnet:master Jul 19, 2020
@qmfrederik qmfrederik deleted the features/deviceiocontrol-nativeoverlapped branch July 20, 2020 08:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review and Improve Overlapped IO Policies
2 participants