Skip to content

Conversation

@jack-pappas
Copy link

I've created H5PT and H5LT classes with P/Invoke signatures for their respective HDF5 APIs (from hdf5-hl).

I've marked this as a work-in-progress (WIP) in the PR title for now, as I still need to implement some unit tests for the APIs.

@gheber
Copy link
Member

gheber commented Nov 20, 2017

@jack-pappas thanks for this pull request. The reason why we left H5PT & co. out of HDF.PInvoke is that they are not thread-safe. Maybe there's a better solution, be we might consider packaging/distributing them in separate assemblies. What do you think?

@jack-pappas
Copy link
Author

@gheber I'd be fine including the high-level APIs in a separate assembly (e.g. HDF.PInvoke.HighLevel), but I think that assembly/project should still live in this repo. Regarding whether to distribute it within the same NuGet package or not -- I think the argument could go both ways. I'd lean towards putting them in the same package though, since hdf5_hl.dll is already being included with the other native binaries (IIRC, but it's been a few months since I looked).

One thing that comes to mind though -- the low-level APIs aren't thread-safe either, unless HDF5 is compiled with thread-safety enabled. Even if that's how the binaries are being compiled for distribution as part of the NuGet packages (are they?), I think the wrappers here should still be agnostic to that -- users of this package may just want the HDF.PInvoke assembly to use with HDF5 binaries they've compiled themselves (for one reason or another). Given this library is meant to be a non-opinionated, zero-overhead wrapper around HDF5, maybe it's best not to design the wrapper around the thread-safety (or not) of the underlying APIs and leave that up to application developers to handle just as they would if e.g. writing a C++ library and linking it to hdf5.dll.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants