Skip to content

Replace String with Symbol where possible - #80091

Merged
bors merged 1 commit into
rust-lang:masterfrom
GuillaumeGomez:str-to-symbol
Dec 17, 2020
Merged

Replace String with Symbol where possible#80091
bors merged 1 commit into
rust-lang:masterfrom
GuillaumeGomez:str-to-symbol

Conversation

@GuillaumeGomez

@GuillaumeGomezGuillaumeGomez commented Dec 16, 2020

Copy link
Copy Markdown
Member

The same as #80047 but on different types. Might be interesting to run some perf comparison.

r? @jyn514

@rust-highfive

rust-highfive commented Dec 16, 2020

Copy link
Copy Markdown
Contributor

Some changes occurred in intra-doc-links.

cc @jyn514

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 16, 2020
@jyn514jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 16, 2020
jyn514
jyn514 reviewed Dec 16, 2020
View reviewed changes

@jyn514jyn514 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.

This looks amazing, thanks so much ❤️

Comment threadsrc/librustdoc/clean/auto_trait.rs Outdated
Comment threadsrc/librustdoc/clean/mod.rs Outdated
Comment threadsrc/librustdoc/clean/mod.rs Outdated
Comment threadsrc/librustdoc/clean/mod.rs Outdated
Comment threadsrc/librustdoc/clean/types.rs Outdated
Comment threadsrc/librustdoc/html/format.rs Outdated
Comment threadsrc/librustdoc/html/format.rs Outdated
Comment threadsrc/librustdoc/html/format.rs Outdated
Comment threadsrc/librustdoc/html/render/cache.rs Outdated
Comment threadsrc/librustdoc/passes/collect_intra_doc_links.rs Outdated
Comment on lines 756 to 764

@jyn514jyn514Dec 16, 2020

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.

This is weird, not sure why it changed :/ did you run the wrong version of rustfmt maybe?

@GuillaumeGomezGuillaumeGomezDec 16, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No idea. I always run my "own" version, which isn't the best idea I think.

@jyn514jyn514Dec 16, 2020

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.

If you run x.py fmt it should work correctly.

@GuillaumeGomez

GuillaumeGomez commented Dec 16, 2020

Copy link
Copy Markdown
MemberAuthor

Updated! If it looks fine to you, I'll let you start the perf check. :3

@GuillaumeGomez

GuillaumeGomez commented Dec 16, 2020

Copy link
Copy Markdown
MemberAuthor

Sorry, can't wait, just want to know! ⏩

@bors try @rust-timer queue

@rust-timer

rust-timer commented Dec 16, 2020

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion.

@bors

bors commented Dec 16, 2020

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 50d221c with merge ed385fcba33dd70047504a3e0c0aadd0106d7e5a...

@bors

bors commented Dec 16, 2020

Copy link
Copy Markdown
Collaborator

☀️ Try build successful - checks-actions
Build commit: ed385fcba33dd70047504a3e0c0aadd0106d7e5a (ed385fcba33dd70047504a3e0c0aadd0106d7e5a)

@rust-timer

rust-timer commented Dec 16, 2020

Copy link
Copy Markdown
Collaborator

Queued ed385fcba33dd70047504a3e0c0aadd0106d7e5a with parent 2ba7ca2, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 16, 2020
@rust-timer

rust-timer commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

Finished benchmarking try commit (ed385fcba33dd70047504a3e0c0aadd0106d7e5a): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 17, 2020
@jyn514

jyn514 commented Dec 17, 2020

Copy link
Copy Markdown
Member

Wow! -4.7% on instructions, -1.3% on max-rss :) Up to -6.2% on wall-times, although that's always variable.

@jyn514

jyn514 commented Dec 17, 2020

Copy link
Copy Markdown
Member

@bors r+ rollup=never

@bors

bors commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 50d221c has been approved by jyn514

@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 Dec 17, 2020
@bors

bors commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 50d221c with merge bdd0a78...

@bors

bors commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing bdd0a78 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Dec 17, 2020
@bors
bors merged commit bdd0a78 into rust-lang:masterDec 17, 2020
@rustbotrustbot added this to the 1.50.0 milestone Dec 17, 2020
@borsbors mentioned this pull request Dec 17, 2020
Merged
@GuillaumeGomez
GuillaumeGomez deleted the str-to-symbol branch December 17, 2020 10:31
@GuillaumeGomezGuillaumeGomez mentioned this pull request Dec 17, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2020
@bors
Auto merge ofrust-lang#80119- GuillaumeGomez:str-to-symbol, r=jyn514
fee693d
Continue String to Symbol conversion in rustdoc
Follow-up of rust-lang#80091.
This PR is already big enough so I'll stop here before the next one.
r? `@jyn514`
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@jyn514jyn514jyn514 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@jyn514jyn514

Labels

C-cleanupCategory: PRs that clean code up or issues documenting cleanup.I-compilememIssue: Problems and improvements with respect to memory usage during compilation.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.T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.50.0

Development

Successfully merging this pull request may close these issues.

6 participants

@GuillaumeGomez@rust-highfive@rust-timer@bors@jyn514@rustbot