support for windows #26
Conversation
src/lib.rs
Outdated
| #[path = "windows.rs"] | ||
| mod sys; | ||
|
|
||
| #[cfg(not(any(target_os = "linux", target_os = "macos", target_os = "windows", target_family = "wasm")))] |
There was a problem hiding this comment.
As this grows more complicated, can the cfg_if crate be used to avoid a blowup of conditions here?
src/lib.rs
Outdated
| #[macro_use] | ||
| extern crate once_cell; | ||
| #[cfg(target_os = "windows")] | ||
| extern crate windows; |
There was a problem hiding this comment.
I think neither of these should be necessary with Rust 2018
src/windows.rs
Outdated
|
|
||
| #[cfg(feature = "global")] | ||
| static LOCK: Lazy<windows::Win32::Foundation::HANDLE> = unsafe { | ||
| Lazy::new(|| { |
There was a problem hiding this comment.
Given how low-level this crate is I think it would be best to not use an external crate to perform this initialization and instead internally use locking/synchronization as necessary.
There was a problem hiding this comment.
The thing is that I actually tried to initialize the lock without relying on external crate, but rust compiler didn't allow me to proceed displaying the following error message: calls in statics are limited to constant functions, tuple structs and tuple variants. So, I was advised to lazy-initialize the lock with the help of this crate. I'd be glad if you suggest an alternative
There was a problem hiding this comment.
Sorry I don't know how best to fix that, but regardless I don't think that using an external crate is appropriate in this case for a lock around the allocator.
There was a problem hiding this comment.
I think the best way to do this is to set the lock to Handle(0) at first. Then, in the lock functions, check if its equal to that and create the mutex if it is.
.github/workflows/main.yml
Outdated
| @@ -20,6 +20,22 @@ jobs: | |||
| - run: RUSTFLAGS='--cfg test_lots' cargo test --release | |||
| - run: RUSTFLAGS='--cfg test_lots' cargo test --release --features debug | |||
|
|
|||
| windows-test: | |||
| name: Test-windows | |||
| runs-on: windows-latest | |||
There was a problem hiding this comment.
Can this be added to the matrix above instead of duplicating the CI config?
Windows support: Rebase #26
|
I rebased this in #35 and handled some comments of mine, thanks for the PR! |
I just thought that it wouldn't hurt to add support for windows systems.