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.

Sign up for GitHub

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

Jump to bottom

More const {} init in thread_local #137194

Merged
merged 3 commits into from
Feb 23, 2025
Merged

More const {} init in thread_local #137194

merged 3 commits into from
Feb 23, 2025

Conversation

kornelski
Copy link
Contributor

const {} in thread_local! gets an optimization just based on the syntax, rather than the expression being const-compatible. This is easy to miss, so I've added more examples to the docs.

I've also added const {} in a couple of places in std where this optimization has been missed.

@rustbot
Copy link
Collaborator

rustbot commented Feb 17, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 17, 2025
@workingjubilee
Copy link
Member

was 40cf00a meant to be part of this PR? (not particularly that I have an issue with it, just curious since it's not obvious why this PR would come with that particular test).

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@workingjubilee
Copy link
Member

...well, okay, to be clear bors is going to take issue with it so you probably can't get this merged with that commit

@tgross35
Copy link
Contributor

I think that commit is included because current_thread_id is one of the functions changed to make use of const, in the third commit. But the test could definitely use a comment about what it is intended to test.

@tgross35
Copy link
Contributor

@rustbot author for the above

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2025
@kornelski
Copy link
Contributor Author

I've put the test in the same commit it tests, and added a comment why. ad56664

@tgross35
Copy link
Contributor

As far as I know, thread locals must appear as different addresses when accessed from different threads, so whether a constant or lazy initializer was used shouldn't make a difference here. The sanity check certainly doesn't hurt though.

(I'm going to double check with opsem to make sure this is accurate)

Thanks for the changes!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 23, 2025

📌 Commit 4742dbc has been approved by tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 23, 2025
@jhpratt jhpratt mentioned this pull request Feb 23, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2025
@bors
Auto merge of rust-lang#137466 - jhpratt:rollup-spyi02y, r=jhpratt
1805b33 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#135354 ([Debuginfo] Add MSVC Synthetic and Summary providers to LLDB)
 - rust-lang#136826 (Replace mem::zeroed with mem::MaybeUninit::uninit for large struct in Unix)
 - rust-lang#137194 (More const {} init in thread_local)
 - rust-lang#137334 (Greatly simplify lifetime captures in edition 2024)
 - rust-lang#137382 (bootstrap: add doc for vendor build step)
 - rust-lang#137423 (Improve a bit HIR pretty printer)
 - rust-lang#137435 (Fix "missing match arm body" suggestion involving `!`)
 - rust-lang#137448 (Fix bugs due to unhandled `ControlFlow` in compiler)
 - rust-lang#137458 (Fix missing self subst when rendering `impl Fn*<T>` with no output type)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2ff53a2 into rust-lang:master Feb 23, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 23, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2025
@rust-timer
Unrolled build for rust-lang#137194
712b787 
Rollup merge of rust-lang#137194 - kornelski:ftls, r=tgross35

More const {} init in thread_local

`const {}` in `thread_local!` gets an optimization just based on the syntax, rather than the expression being const-compatible. This is easy to miss, so I've added more examples to the docs.

I've also added `const {}` in a couple of places in std where this optimization has been missed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35 tgross35 approved these changes

Assignees

@tgross35 tgross35

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
6 participants
@kornelski @rustbot @workingjubilee @rust-log-analyzer @tgross35 @bors