dotnet / pinvoke

A library containing all P/Invoke code so you don't have to import it every time. Maintained and updated to support the latest Windows OS.
MIT License
2.12k stars 222 forks source link

Help me merge my library into PInvoke #261

Closed josemesona closed 4 years ago

josemesona commented 8 years ago

I have a similar P/Invoke library that I'd like to deprecate in favor of yours but there a few things missing.

  1. CopyProgressRoutine - My implementation is here I think this should be straightforward but I'm not sure where it should go, probably Windows.Core?
  2. Extension methods for marshaling pointers
    1. Marshal a pointer to an array of pointers to an IEnumerable
    2. Marshal a managed collection into a pointer to an array of pointers
    3. Marshal a pointer to a string
  3. Implicit operators for some structs like this

Are you okay with me contributing this stuff and if so, do you have some guidance where they should go?

Thanks

AArnott commented 8 years ago

Thanks, @josemesona. Yes, the CopyProgressRoutine function should go into Windows.Core, based on it being defined in windows.h. Your class that makes the progress routine more consumable looks great, but may lie outside the scope of this project, which is to just focus on native interop rather than raising the abstraction layer significantly, which can be done in other libraries that consume this one. But I'd be interested in other contributors' thoughts on this (cc: @vbfox).

Regarding the extension methods, I'm hesitant. The enumerator conversion makes me wonder if it could lead to memory leaks (you never call DestroyStructure nor is there really a good time to do that given the pattern folks would use this with). And the others just forward to Marshal which seems to just create another way to do something already fairly simple.

It looks like we're missing SYSTEMTIME that you have. That and other P/Invoke signatures you may have would be great additions. I think the implicit operators between SYSTEMTIME and DateTime would be welcome too, provided no data loss in the conversion takes place.

vbfox commented 8 years ago

I generally agree on what @AArnott said:

For the 2 first points they cover a need that is a little out of scope of the lib but are still interesting contributions. Maybe at some point we can start a separate PInvoke.Contrib repo with code like that that isn't purely related to the core of PInvoke but still useful.

arlm commented 8 years ago

@josemesona, I can help you with the PR if you want, taking into account @vbfox and @AArnott suggestions and comments. If you want, you can PR on my fork (arlm/pinvoke), on the develop branch.

About the higher level functions like the CopyProgressRoutine, if you like, you could PR it to a higher level API mapping project I started (WinApi), built upon this project, to place the out of scope mappings I needed to use and wanted to share. If you want to, you could place a new file called FileApi.cs under WinApi.Core project for now but the ideal would be using the template\AddNewLibrary.ps1 script to create WinApi.FileApi which is the API set this function lies under (just pass through FileApi as parameter). The Find, Copy, Link Files higher level mappings could go into that library also, given that the low-level PInvoke comes here on the appropriate place.

The service methods like ChangeServiceConfig, SetServiceStartModeand ServiceConfig would be better placed under a new WinApi.ServiceApi project and the low-level API would go under PInvoke.Advapi32 project.

The IntPtr.To* extensions could go on WinApi.Core@MarshallingExtensions if you like, but I also would not use the generic enumerable extensions and have one for each specialization instead, of course, paying attention to the quirks and tricks of the specific API-set.

I think that if you want to go this way we could continue the merge/port discussion on the appropriate repositories issues instead of here, not to clutter it. Feel free to do as you want.

AArnott commented 8 years ago

I love the synergy and collaboration. I look forward to seeing WinApi thrive. We should consider advertising it in this project's README, if @arlm thinks it's ready for prime time.

josemesona commented 8 years ago

Sorry for the delay here. @AArnott, @vbfox, and @arlm, thanks for the input.

I definitely understand trying to keep this project scoped. My intention with the Microsoft.Win32Ex library was to fill the gap of missing functionality with as much extension methods as possible hoping that one day Microsoft would add this stuff to .NET. There's a mix of P/Invoke signatures and helpers that make my other projects easier. When I came across this set of libraries I saw a lot more implemented than I have and really wanted to deprecate mine so there's just one good library out there for interacting with Native functionality that isn't available in .NET. If some of the stuff is rolled into the P/Invoke libraries and not others, then it is just going to add more dependencies for my end use projects which is not ideal. I'm thinking about running ILMerge in my libraries so that I can ship a single assembly. However, that has obvious drawbacks as well.

I'll start porting the stuff I need via @arlm 's fork and go from there. Hopefully I can just get rid of my library in the end.

Thanks

AArnott commented 8 years ago

@josemesona see #106 for our last look at ILMerge for this project. But if you do something clever, please share on that issue in case it's relevant to this project. :)