Skip to content

Library::open_already_loaded() for Windows #85

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 14, 2021

Conversation

YaLTeR
Copy link
Contributor

@YaLTeR YaLTeR commented Dec 30, 2020

Fixes the Windows half of #27.

For unix I guess I can make a cfg-gated function? Not sure which platforms though. Also, if so, should I also add a cfg-gated top-level Library function?

@YaLTeR YaLTeR changed the title Windows open already loaded Library::open_already_loaded() for Windows Dec 30, 2020
@YaLTeR YaLTeR force-pushed the windows-open-already-loaded branch from 79ed158 to 299d79d Compare December 30, 2020 16:45
@nagisa
Copy link
Owner

nagisa commented Dec 30, 2020

For unix I guess I can make a cfg-gated function?

So far I have avoided adding cfg-gated APIs that are not obviously non-portable at a glance. use os::platform makes it obvious for unix vs windows non-portability and is the same scheme that the standard library uses so it is familiar to both developers and reviewers. It would be a shame if we had an API in os::unix that didn't work for all unix targets.

There's an RFC for portability lint for a more general and less involving approach, but it hasn't been implemented. Until it, or something similar to it, is implemented, libloading should probably refrain from adding such APIs.

While we're waiting users of the library can still grab RTLD_NOLOAD from libc and pass it to os::unix::Library::open.

@YaLTeR
Copy link
Contributor Author

YaLTeR commented Jan 14, 2021

So, any other changes? Bikeshedding for the method name?

@nagisa
Copy link
Owner

nagisa commented Jan 14, 2021

Ah yeah, thanks for the ping, I had forgotten about this entirely!

@nagisa nagisa merged commit 299d79d into nagisa:master Jan 14, 2021
@YaLTeR
Copy link
Contributor Author

YaLTeR commented Jan 14, 2021

Cheers!

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