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

to_owned should be used in favor of to_string #29

Merged
merged 1 commit into from
Oct 25, 2015

Conversation

leaexplores
Copy link
Contributor

Hi, this PR stumbles upon a small nit I covered while seeking some code in cocoa-rs and servo.

It fixes the to_string calls for to_owned. It is much lighter in resources.

To_string as uses the whole formatting machinery just to clone a string.

Thanks for looking into it !

@leaexplores
Copy link
Contributor Author

ping? @SSheldon

@SSheldon
Copy link
Owner

SSheldon commented Oct 5, 2015

Hey @ddrmanxbxfr, this looks fine but I'm curious if to_owned is now the most idiomatic way to do this conversion. In rust-lang/rust#26176, some of the rust core team claims to_string is more idiomatic and semantically correct.

Since into_string was removed, I'd been under the impression that to_string was the correct way to convert a str into a String, and that it would eventually be optimized to avoid the cost.

Do you think this code is more idiomatic with to_owned? Or, if not, does the performance improvement here seem worth it anyways?

@SSheldon
Copy link
Owner

Ping @ddrmanxbxfr! I could use your help here figuring out what's the best way to convert to a String :)

@leaexplores
Copy link
Contributor Author

Hi @SSheldon my bad I was PTO for a few days.

to_string does feel more indiomatic and semantically correct.

However as of right now to_string does come with a runtime overhead and to_owned should be used in this case.

Here's an example issue : rust-lang/rust#18404

Rust clippy (a rust linter) also checks to use to_owned in favor of to_string : https://github.com/Manishearth/rust-clippy/wiki#str_to_string

May we see a merge between to_owned and to_string when to_string will be up par the performance level of to_owned.

@SSheldon
Copy link
Owner

Oh I didn't realize rust-clippy had a lint for this, that's a pretty strong point in favor of to_owned. Thanks, merging this!

@SSheldon SSheldon merged commit 1c3f102 into SSheldon:master Oct 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants