Skip to content

Add Windows InitOnceBeginInitialize and InitOnceComplete shims - #2601

Merged
bors merged 8 commits into
rust-lang:masterfrom
beepster4096:windows_init_once
Oct 20, 2022
Merged

bors merged 8 commits into
rust-lang:masterfrom
beepster4096:windows_init_once

Conversation

@beepster4096

Copy link
Copy Markdown
Contributor

Fixes #2595

Comment thread src/concurrency/sync.rs Outdated
Comment thread src/shims/windows/sync.rs Outdated
@beepster4096

Copy link
Copy Markdown
Contributor Author

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

Comment thread rust-version Outdated

@RalfJung RalfJung left a comment

Copy link
Copy Markdown
Member

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!

Comment thread src/concurrency/sync.rs Outdated
Comment thread src/concurrency/sync.rs Outdated
Comment thread src/concurrency/sync.rs Outdated
Comment thread src/concurrency/sync.rs Outdated
Comment thread src/concurrency/sync.rs Outdated
Comment thread src/concurrency/sync.rs Outdated
Comment thread src/concurrency/sync.rs Outdated
Comment thread src/shims/unix/sync.rs Outdated
Comment thread src/shims/windows/sync.rs Outdated
Comment on lines +147 to +148
InitOnceStatus::Begun => this.eval_windows("c", "TRUE")?,
InitOnceStatus::Complete => this.eval_windows("c", "FALSE")?,

Copy link
Copy Markdown
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 thread src/shims/windows/sync.rs Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

@bors r+

@bors

bors commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

📌 Commit c9b32cc has been approved by RalfJung

It is now in the queue for this repository.

@bors

bors commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

⌛ Testing commit c9b32cc with merge 64757ed...

@bors

bors commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

☀️ 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