Skip to content
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

Add Windows InitOnceBeginInitialize and InitOnceComplete shims #2601

Merged
merged 8 commits into from
Oct 20, 2022

Conversation

beepster4096
Copy link
Contributor

Fixes #2595

src/concurrency/sync.rs Outdated Show resolved Hide resolved
src/shims/windows/sync.rs Outdated Show resolved Hide resolved
@beepster4096
Copy link
Contributor Author

I think rust-lang/rust#103198 should fix the CI failure when its merged.

rust-version Outdated Show resolved Hide resolved
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this mostly looks great!

src/concurrency/sync.rs Outdated Show resolved Hide resolved
src/concurrency/sync.rs Outdated Show resolved Hide resolved
src/concurrency/sync.rs Outdated Show resolved Hide resolved
src/concurrency/sync.rs Outdated Show resolved Hide resolved
src/concurrency/sync.rs Outdated Show resolved Hide resolved
src/concurrency/sync.rs Outdated Show resolved Hide resolved
src/concurrency/sync.rs Outdated Show resolved Hide resolved
src/shims/unix/sync.rs Outdated Show resolved Hide resolved
Comment on lines 147 to 148
InitOnceStatus::Begun => this.eval_windows("c", "TRUE")?,
InitOnceStatus::Complete => this.eval_windows("c", "FALSE")?,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I didn't know about these constants. But we should be consistent -- I think all the other Windows shims use Scalar::from_i32 to create their BOOL literals. If we're going with eval for this, can you make a separate PR changing the existing code?

Comment on lines 163 to 167
this.init_once_enqueue_and_block(
id,
active_thread,
Box::new(Callback { init_once_id: id, pending_place }),
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the callback into this match arm. It is confusing to read all that callback code and then realize it is only sometimes relevant.

@RalfJung
Copy link
Member

In the interest of having our CI fixed, I plan to take over this PR later today so that it can land ASAP. I hope that's okay.

@RalfJung RalfJung force-pushed the windows_init_once branch 2 times, most recently from 03da888 to 1413857 Compare October 20, 2022 21:09
@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2022

📌 Commit c9b32cc has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 20, 2022

⌛ Testing commit c9b32cc with merge 64757ed...

@bors
Copy link
Contributor

bors commented Oct 20, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 64757ed to master...

@bors bors merged commit 64757ed into rust-lang:master Oct 20, 2022
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.

can't call foreign function: InitOnceBeginInitialize when running on Windows
5 participants