Skip to content

Conversation

ljedrz
Copy link
Contributor

@ljedrz ljedrz commented Sep 3, 2018

  • calculate the capacity of some Vecs
  • changeto_owned() to clone() for the purposes of lower_attributes
  • remove a superfluous clone()
  • prefer to_owned() to to_string()
  • a few other minor improvements

@rust-highfive
Copy link
Contributor

r? @oli-obk

(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 Sep 3, 2018
Copy link
Contributor

Choose a reason for hiding this comment

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

the ampersand is unecessary

Copy link
Contributor

Choose a reason for hiding this comment

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

seems weird to go from clone to to_owned. One wouldn't call to_owned on a &String, only on a &str I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both variants are currently present in the file and I thought that to_owned() would be preferable here, but I don't mind performing the change the other way around.

@ljedrz ljedrz force-pushed the save_analysis_cleanups branch from d14573d to 9883dd7 Compare September 3, 2018 16:38
@ljedrz
Copy link
Contributor Author

ljedrz commented Sep 3, 2018

Thanks for the review, comments addressed.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 3, 2018

@bors r+ rollup

Thx for the quick fix

@bors
Copy link
Collaborator

bors commented Sep 3, 2018

📌 Commit 9883dd7 has been approved by oli-obk

@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 Sep 3, 2018
@bors
Copy link
Collaborator

bors commented Sep 4, 2018

⌛ Testing commit 9883dd7 with merge 8b80390...

bors added a commit that referenced this pull request Sep 4, 2018
A few cleanups and minor improvements to save_analysis

- calculate the capacity of some `Vec`s
- change`to_owned()` to `clone()` for the purposes of `lower_attributes`
- remove a superfluous `clone()`
- prefer `to_owned()` to `to_string()`
- a few other minor improvements
@bors
Copy link
Collaborator

bors commented Sep 4, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 8b80390 to master...

@bors bors merged commit 9883dd7 into rust-lang:master Sep 4, 2018
@ljedrz ljedrz deleted the save_analysis_cleanups branch September 4, 2018 06:47
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants