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

use String::from() instead of format!() macro to construct Strings. #55522

Merged
merged 1 commit into from
Nov 1, 2018
Merged

use String::from() instead of format!() macro to construct Strings. #55522

merged 1 commit into from
Nov 1, 2018

Conversation

matthiaskrgr
Copy link
Member

No description provided.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Oct 31, 2018
@zackmdavis
Copy link
Member

r? @zackmdavis

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 31, 2018

📌 Commit f6b8876 has been approved by zackmdavis

@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 Oct 31, 2018
@ExpHP
Copy link
Contributor

ExpHP commented Oct 31, 2018

What's the point of this? All of these format!s are next to other format!s, so they benefit from consistency; and they're all in cold code paths, so the performance gains are null.

@pietroalbini pietroalbini mentioned this pull request Nov 1, 2018
Merged
kennytm pushed a commit to pietroalbini/rust that referenced this pull request Nov 1, 2018
@pietroalbini @kennytm
Rollup merge of rust-lang#55522 - matthiaskrgr:no_format, r=zackmdavis
1e1d37b 
use String::from() instead of format!() macro to construct Strings.
bors added a commit that referenced this pull request Nov 1, 2018
@bors
Auto merge of #55579 - pietroalbini:rollup, r=kennytm
451987d 
Rollup of 13 pull requests

Successful merges:

 - #55280 (Add libproc_macro to rust-src distribution)
 - #55469 (Regression tests for issue #54477.)
 - #55504 (Use vec![x; n] instead of iter::repeat(x).take(n).collect())
 - #55522 (use String::from() instead of format!() macro to construct Strings.)
 - #55536 (Pass suggestions as impl Iterator instead of Vec)
 - #55542 (syntax: improve a few allocations)
 - #55558 (Tweak `MatcherPos::matches`)
 - #55561 (Fix double_check tests on big-endian targets)
 - #55573 (Make sure the `aws` executable is in $PATH on macOS)
 - #55574 (Use `SmallVec` within `MoveData`.)
 - #55575 (Fix invalid_const_promotion test on some archs)
 - #55578 (Made doc example of `impl Default for …` use `-> Self` instead of explicit self type)
 - #55582 (Remove unused import copy from publish_toolstate.py)
@bors bors merged commit f6b8876 into rust-lang:master Nov 1, 2018
@RalfJung
Copy link
Member

RalfJung commented Nov 2, 2018 •
edited
Loading

Is this an efficiency question or a style question? (For the future, it'd be great if such questions could be answered in the PR description or commit message, which seem like a likely place someone will look if they want to find out why something got changed.)

If it is about efficiency, is there any reason format! on a string literal has to be less efficient? Seems easy enough to special-case in a macro.

If it is about style, I'd prefer if you could consider consistency: In the miri validator (librustc_mir/interpret/validity.rs), we generate quite a few error messages, and we consistently do that with format! as many of them need some formatting. Exactly one doesn't, and you changed it to String::from. Now this one error message stands out in the code because it is not consistent with what we do everywhere else, that looks really odd. String::from is also longer, and this this case I do not think it increases readability, either. I hope you don't mind that when I resolved a merge conflict between this PR and mine, I changed this back to format!.

EDIT: Oh, just saw @ExpHP raised the same question. :)

@zackmdavis
Copy link
Member

@ExpHP @RalfJung I can't speak to @matthiaskrgr's intent, but I felt comfortable approving the PR on style grounds: note that Clippy has a warn-by-default useless_format lint. I'm optimistic about eventually making it easy to run Clippy/rustfmt/&c. in this repo (#53896) to minimize the amount of contributor/reviewer effort spent on style-wrangling.

@matthiaskrgr matthiaskrgr deleted the no_format branch February 29, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@zackmdavis zackmdavis

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
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@matthiaskrgr @rust-highfive @zackmdavis @bors @ExpHP @RalfJung @matthewjasper