Skip to content

More static symbols - #74175

Merged
bors merged 4 commits into
rust-lang:masterfrom
nnethercote:more-static-symbols
Jul 15, 2020
Merged

More static symbols#74175
bors merged 4 commits into
rust-lang:masterfrom
nnethercote:more-static-symbols

Conversation

@nnethercote

@nnethercotennethercote commented Jul 9, 2020

Copy link
Copy Markdown
Contributor

These commits add some more static symbols and convert lots of places to use them.

r? @oli-obk

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 9, 2020
@nnethercote

nnethercote commented Jul 9, 2020

Copy link
Copy Markdown
ContributorAuthor

Eh, I have to rebase.

oli-obk
oli-obk reviewed Jul 9, 2020
View reviewed changes
Comment threadsrc/librustc_builtin_macros/deriving/generic/mod.rs Outdated
oli-obk
oli-obk reviewed Jul 9, 2020
View reviewed changes
Comment threadsrc/librustc_builtin_macros/global_allocator.rs Outdated
oli-obk
oli-obk reviewed Jul 9, 2020
View reviewed changes
Comment threadsrc/librustc_hir/lang_items.rs Outdated
oli-obk
oli-obk reviewed Jul 9, 2020
View reviewed changes
Comment threadsrc/librustc_hir/lang_items.rs Outdated
oli-obk
oli-obk reviewed Jul 9, 2020
View reviewed changes
Comment threadsrc/test/ui/hygiene/unpretty-debug.stdout Outdated
petrochenkov
petrochenkov reviewed Jul 9, 2020
View reviewed changes
Comment threadsrc/librustc_span/symbol.rs Outdated
Comment threadsrc/librustc_span/symbol.rs Outdated
@nnethercote

nnethercote commented Jul 9, 2020

Copy link
Copy Markdown
ContributorAuthor

Thanks for the quick feedback! I have rebased and addressed the comments.

oli-obk
oli-obk reviewed Jul 9, 2020
View reviewed changes
Comment threadsrc/librustc_ast/expand/allocator.rs Outdated
@oli-obk

oli-obk commented Jul 9, 2020

Copy link
Copy Markdown
Contributor

r=me with the as_str removed

@nnethercote

nnethercote commented Jul 9, 2020

Copy link
Copy Markdown
ContributorAuthor

r=me with the as_str removed

What do you think about my explanation?

@nnethercote
nnethercoteforce-pushed the more-static-symbols branch 2 times, most recently from ed89e7c to 7e492d4CompareJuly 9, 2020 12:27
@nnethercote

nnethercote commented Jul 9, 2020

Copy link
Copy Markdown
ContributorAuthor

@bors r=oli-obk

@bors

bors commented Jul 9, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 7e492d4efc878f20ac18deaeed5806fe8895cfb8 has been approved by oli-obk

@borsbors 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

nnethercote commented Jul 9, 2020

Copy link
Copy Markdown
ContributorAuthor

@bors p=1

Because this is conflict-prone.

@Mark-Simulacrum

Mark-Simulacrum commented Jul 9, 2020

Copy link
Copy Markdown
Member

Is this potentially affecting perf? Should we avoid rolling it up?

@oli-obk

oli-obk commented Jul 9, 2020

Copy link
Copy Markdown
Contributor

@bors rollup=never

while I consider it unlikely to affect perf, the potential is there.

@nnethercotennethercote mentioned this pull request Jul 10, 2020
Merged
@nnethercote

nnethercote commented Jul 10, 2020

Copy link
Copy Markdown
ContributorAuthor

I did some local measurements of check runs (which should be representative) and the perf effects were negligible. But the changes would be much too big for a rollup anyway, I would have thought?

@oli-obk

oli-obk commented Jul 10, 2020

Copy link
Copy Markdown
Contributor

I did some local measurements of check runs (which should be representative) and the perf effects were negligible. But the changes would be much too big for a rollup anyway, I would have thought?

There's a small (but important) difference between marking a PR for rollups and including a PR in rollups.

The latter happens frequently and for all PRs except for perf-related PRs.

@nnethercote

nnethercote commented Jul 10, 2020

Copy link
Copy Markdown
ContributorAuthor

I heard recently that changes above a particular size were not considered for rollups. Is that incorrect?

@oli-obk

oli-obk commented Jul 10, 2020

Copy link
Copy Markdown
Contributor

Hmm... I honestly don't know our rollup policy that well. But 🤷 it's rollup=never now anyway, so gonna be fine.

@Manishearth

Manishearth commented Jul 10, 2020

Copy link
Copy Markdown
Member

@bors rollup=never

@Manishearth

Manishearth commented Jul 10, 2020

Copy link
Copy Markdown
Member

@bors p=0

rollup=never now autoprioritizes

@Manishearth

Manishearth commented Jul 14, 2020

Copy link
Copy Markdown
Member

@bors retry

  • network

@borsbors 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 14, 2020
@bors

bors commented Jul 14, 2020

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #74330) made this pull request unmergeable. Please resolve the merge conflicts.

@borsbors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 14, 2020
@Manishearth

Manishearth commented Jul 14, 2020

Copy link
Copy Markdown
Member

Sorry about that, once you patch this up I'll try to get this landed.

@bors p=1

@nnethercote
Rename sym::nontrapping_fptoint.
026089b
@nnethercote
Fix the ordering of the static symbols.
e284f5d
@nnethercote
Add and use more static symbols.
f04e866
Note that the output of `unpretty-debug.stdout` has changed. In that
test the hash values are normalized from a symbol numbers to small
numbers like "0#0" and "0#1". The increase in the number of static
symbols must have caused the original numbers to contain more digits,
resulting in different pretty-printing prior to normalization.
@nnethercote
Remove lots of Symbol::as_str() calls.
5930081
In various ways, such as changing functions to take a `Symbol` instead
of a `&str`.
@nnethercote

nnethercote commented Jul 14, 2020

Copy link
Copy Markdown
ContributorAuthor

@bors r=oli-obk

@bors

bors commented Jul 14, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 5930081 has been approved by oli-obk

@bors

bors commented Jul 14, 2020

Copy link
Copy Markdown
Collaborator

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

@borsbors 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 Jul 14, 2020
@bors

bors commented Jul 15, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 5930081 with merge 567ad74...

@borsbors mentioned this pull request Jul 15, 2020
Merged
@bors

bors commented Jul 15, 2020

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 567ad74 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Jul 15, 2020
@bors
bors merged commit 567ad74 into rust-lang:masterJul 15, 2020
This was referenced Jul 15, 2020
Merged
Merged
Merged
Closed
Merged
Merged
@nnethercote
nnethercote deleted the more-static-symbols branch July 15, 2020 04:20
@cuvipercuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obkoli-obkoli-obk left review comments
@petrochenkovpetrochenkovpetrochenkov left review comments

Assignees

@oli-obkoli-obk

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.47.0

Development

Successfully merging this pull request may close these issues.

8 participants

@nnethercote@oli-obk@bors@Mark-Simulacrum@Manishearth@petrochenkov@cuviper@rust-highfive