Skip to content

Prefer to_string() to format!()#52767

Merged
bors merged 1 commit intorust-lang:masterfrom
ljedrz:avoid_format
Jul 29, 2018
Merged

Prefer to_string() to format!()#52767
bors merged 1 commit intorust-lang:masterfrom
ljedrz:avoid_format

Conversation

@ljedrz
Copy link
Copy Markdown
Contributor

@ljedrz ljedrz commented Jul 27, 2018

Simple benchmarks suggest in some cases it can be faster by even 37%:

test converting_f64_long  ... bench:         339 ns/iter (+/- 199)
test converting_f64_short ... bench:         136 ns/iter (+/- 34)
test converting_i32_long  ... bench:          87 ns/iter (+/- 16)
test converting_i32_short ... bench:          87 ns/iter (+/- 49)
test converting_str       ... bench:          54 ns/iter (+/- 15)
test formatting_f64_long  ... bench:         349 ns/iter (+/- 176)
test formatting_f64_short ... bench:         145 ns/iter (+/- 14)
test formatting_i32_long  ... bench:          98 ns/iter (+/- 14)
test formatting_i32_short ... bench:          93 ns/iter (+/- 15)
test formatting_str       ... bench:          86 ns/iter (+/- 23)

@rust-highfive
Copy link
Copy Markdown
Contributor

rust-highfive commented Jul 27, 2018

r? @petrochenkov

(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 Jul 27, 2018
@killercup
Copy link
Copy Markdown
Contributor

killercup commented Jul 27, 2018

Most likely the wrong place to ask this: Can we special-case the format macro for the format!("{}", something) case and expand it to something.to_string() for some known types like &str, String, Cow<str>, and friends? Avoiding this performance trap might be worth the hassle.

@ljedrz
Copy link
Copy Markdown
Contributor Author

ljedrz commented Jul 28, 2018

@killercup the cases you are describing should already be covered by the useless_format clippy lint. I haven't seen one for non-string types implementing Display, but there already seems to be a related issue in the clippy repo.

@killercup
Copy link
Copy Markdown
Contributor

killercup commented Jul 28, 2018 •
edited
Loading

@ljedrz a lint is not the same as making this macro do "the right thing" automatically. But this is a drive-by comment, at best. I'll look for/open a new issue.

@killercup killercup mentioned this pull request Jul 28, 2018
Open
@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented Jul 29, 2018

@bors r+

@bors
Copy link
Copy Markdown
Collaborator

bors commented Jul 29, 2018

📌 Commit 57a5a9b has been approved by petrochenkov

@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 29, 2018
@bors
Copy link
Copy Markdown
Collaborator

bors commented Jul 29, 2018

⌛ Testing commit 57a5a9b with merge 023fd7e...

bors added a commit that referenced this pull request Jul 29, 2018
@bors
Auto merge of #52767 - ljedrz:avoid_format, r=petrochenkov
023fd7e 
Prefer to_string() to format!()

Simple benchmarks suggest in some cases it can be faster by even 37%:
```
test converting_f64_long  ... bench:         339 ns/iter (+/- 199)
test converting_f64_short ... bench:         136 ns/iter (+/- 34)
test converting_i32_long  ... bench:          87 ns/iter (+/- 16)
test converting_i32_short ... bench:          87 ns/iter (+/- 49)
test converting_str       ... bench:          54 ns/iter (+/- 15)
test formatting_f64_long  ... bench:         349 ns/iter (+/- 176)
test formatting_f64_short ... bench:         145 ns/iter (+/- 14)
test formatting_i32_long  ... bench:          98 ns/iter (+/- 14)
test formatting_i32_short ... bench:          93 ns/iter (+/- 15)
test formatting_str       ... bench:          86 ns/iter (+/- 23)
```
@bors
Copy link
Copy Markdown
Collaborator

bors commented Jul 29, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 023fd7e to master...

@bors bors merged commit 57a5a9b into rust-lang:master Jul 29, 2018
@ljedrz ljedrz deleted the avoid_format branch July 29, 2018 11:28
@pnkfelix pnkfelix mentioned this pull request Oct 9, 2018
Closed
@sinkuu sinkuu mentioned this pull request Dec 13, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@petrochenkov petrochenkov

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.

5 participants

@ljedrz @rust-highfive @killercup @petrochenkov @bors