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.

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

Stop using a string literal as a format argument #96248

Merged
merged 1 commit into from
Apr 22, 2022

Conversation

TaKO8Ki
Copy link
Member

@TaKO8Ki TaKO8Ki commented Apr 20, 2022

No description provided.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 20, 2022
@rust-highfive
Copy link
Collaborator

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 20, 2022
@compiler-errors
Copy link
Member

@TaKO8Ki, one line PRs like this are (in my opinion) not significant enough to review... Performance hasn't changed and readability only increased slightly.

Do you mind including changes like this in a more significant PR when you're editing code near by, instead of putting up such small changes independently? Thanks!

@TaKO8Ki
Copy link
Member Author

TaKO8Ki commented Apr 20, 2022

@compiler-errors
I completely agree with you. However, when reading the rustc code, I sometimes find strange programs like this one, so I created this patch for it because I don't feel comfortable leaving it as it is, just like a typo.

@compiler-errors
Copy link
Member

I am just confused why not all of the usages were fixed in this file, or even (if i saw correctly) in this function? I recommend batching at least a few of these into a PR, so we don't have a lot of tiny commits in the logs..

@TaKO8Ki
Copy link
Member Author

TaKO8Ki commented Apr 21, 2022

@compiler-errors
This is because the main point of this pull request is not to use format-args-capture, but to stop using a string literal as a format argument like the following. The former is what I did incidentally.

- format!("{}{}", snippet, ".collect::<Vec<_>>()"),
+ format!("{}.collect::<Vec<_>>()", snippet),

Should I make additional changes like #96065?

@TaKO8Ki TaKO8Ki changed the title Remove an unnecessary format arg Stop using a string literal as a format argument Apr 21, 2022
@compiler-errors
Copy link
Member

I am still not convinced that this change does anything meaningful, but also I will not block it. Thanks for making this contribution -- but also keep this in mind when putting up one-line changes like this that don't affect performance or readability 😅

If you want to change more spots to use format-args-capture, then at least please batch it together, maybe a few crates together at once.

@bors r+ rollup=always

@bors
Copy link
Contributor

bors commented Apr 21, 2022

📌 Commit 5078b05 has been approved by compiler-errors

@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 Apr 21, 2022
@TaKO8Ki
Copy link
Member Author

TaKO8Ki commented Apr 21, 2022

@compiler-errors
I'm sorry. I understand.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 21, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#95434 (Only output DepKind in dump-dep-graph.)
 - rust-lang#96248 (Stop using a string literal as a format argument)
 - rust-lang#96251 (Update books)
 - rust-lang#96269 (errors: minor translation-related changes)
 - rust-lang#96289 (Remove redundant `format!`s)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 18b6ad3 into rust-lang:master Apr 22, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants