Skip to content
New issue

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

#

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

Already on GitHub? # 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.

# 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.
@kornelski kornelski deleted the ftls branch February 25, 2025 15:49
# for free to join this conversation on GitHub. Already have an account? # 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