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

Eliminate confusing "globals" terminology. #74079

Merged
merged 1 commit into from
Jul 10, 2020
Merged

Eliminate confusing "globals" terminology. #74079

merged 1 commit into from
Jul 10, 2020

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jul 6, 2020 •
edited
Loading

There are some structures that are called "globals", but are they global
to a compilation session, and not truly global. I have always found this
highly confusing, so this commit renames them as "session globals" and
adds a comment explaining things.

Also, the commit fixes an unnecessary nesting of set() calls
src/librustc_errors/json/tests.rs

r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2020
@eddyb
Copy link
Member

eddyb commented Jul 6, 2020 •
edited
Loading

Instead of "globals" we could also use "implicit state" or just "state". The concept of "global state" makes sense IMO even if it's scoped to a thread, but we can be more precise than that if it's less confusing overall.

cc @rust-lang/compiler

@petrochenkov petrochenkov self-assigned this Jul 6, 2020
@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 7, 2020 •
edited
Loading

Whether this renaming is worth doing probably depends on the amount of time passing before some "proper design" is implemented. (Year? Perhaps. Month? Hardly.)

I'd prefer to decide which exactly "session" this data is local to and put it into that session's structure, i.e. come up with the "proper design" part sooner rather than later and avoid intermediate renamings.

EDIT: Zulip thread - https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Thread-local.20.60GLOBALS.60

@nnethercote
Copy link
Contributor Author

Whether this renaming is worth doing probably depends on the amount of time passing before some "proper design" is implemented. (Year? Perhaps. Month? Hardly.)

I'm surprised by this response. The change is simple, just a renaming. It's not that invasive?

If nothing else, I definitely want the comments to be added to the Globals structs. It took me ages to understand how they worked because of the misleading name, and I'd like to avoid other people in the future wasting time on it as well.

nikomatsakis
nikomatsakis approved these changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Left a nit, but the renaming seems clearer to me (although I don't personally have any particular association with "global" meaning "to the process", I think I always find it kind of ambiguous just what it's "global" to). I don't know that we need to wait until the one true architecture is found to make small renamings, particularly if there isn't really an active effort towards creating that architecture..? (But maybe there is.)

src/librustc_ast/attr/mod.rs Outdated Show resolved Hide resolved
@petrochenkov petrochenkov removed their assignment Jul 8, 2020
@nnethercote
Copy link
Contributor Author

@bors rollup=always

@nnethercote
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 9, 2020

📌 Commit fdd9840c76dbf5e24494576b0c012cc20866a47c has been approved by nikomatsakis

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2020
@nnethercote
Eliminate confusing "globals" terminology.
81c5bb6 
There are some structures that are called "globals", but are they global
to a compilation session, and not truly global. I have always found this
highly confusing, so this commit renames them as "session globals" and
adds a comment explaining things.

Also, the commit fixes an unnecessary nesting of `set()` calls
`src/librustc_errors/json/tests.rs`
@nnethercote
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 9, 2020

📌 Commit 81c5bb6 has been approved by nikomatsakis

@Manishearth Manishearth mentioned this pull request Jul 9, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 9, 2020
@bors
Auto merge of rust-lang#74195 - Manishearth:rollup-h3m0sl8, r=Manishe…
e59b08e 
…arth

Rollup of 14 pull requests

Successful merges:

 - rust-lang#73292 (Fixing broken link for the Eq trait)
 - rust-lang#73791 (Allow for parentheses after macro intra-doc-links)
 - rust-lang#74070 ( Use for<'tcx> fn pointers in Providers, instead of having Providers<'tcx>.)
 - rust-lang#74077 (Use relative path for local links to primitives)
 - rust-lang#74079 (Eliminate confusing "globals" terminology.)
 - rust-lang#74107 (Hide `&mut self` methods from Deref in sidebar if there are no `DerefMut` impl for the type.)
 - rust-lang#74136 (Fix broken link in rustdocdoc)
 - rust-lang#74137 (Update cargo)
 - rust-lang#74142 (Liballoc use vec instead of vector)
 - rust-lang#74143 (Try remove unneeded ToString import in liballoc slice)
 - rust-lang#74146 (update miri)
 - rust-lang#74150 (Avoid "blacklist")
 - rust-lang#74184 (Add docs for intra-doc-links)
 - rust-lang#74188 (Tweak `::` -> `:` typo heuristic and reduce verbosity)

Failed merges:

 - rust-lang#74122 (Start-up clean-up)
 - rust-lang#74127 (Avoid "whitelist")

r? @ghost
@bors bors merged commit 89c9e97 into rust-lang:master Jul 10, 2020
@nnethercote nnethercote deleted the session-globals branch July 10, 2020 01:30
@petrochenkov petrochenkov mentioned this pull request Jul 26, 2020
Closed
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@Aaron1011 Aaron1011

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.46
Development

Successfully merging this pull request may close these issues.
8 participants
@nnethercote @eddyb @petrochenkov @bors @nikomatsakis @cuviper @Aaron1011 @rust-highfive