Skip to content

change String::from_str() to .to_string() #15519

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

Closed
wants to merge 1 commit into from

Conversation

cburgdorf
Copy link
Contributor

I recently learned from @huonw that "foo".to_string() should be preferred over String::from_str("foo").

This commit updates tutorial.md, rc.rs and string.rs according to the convention.

There are more files to be patched but I thought it might be fine to patch them in batches.

@cburgdorf
Copy link
Contributor Author

Travis seemed to timed out hence the error. Not sure how to trigger a new run.

@lilyball
Copy link
Contributor

lilyball commented Jul 8, 2014

Seems like a poor time to do this migration, when "foo".to_string() is temporarily going to be much more expensive than String::from_str("foo").

@brson
Copy link
Contributor

brson commented Jul 8, 2014

As of #15493 it's not obvious that to_string should be preferred over from_str because there's a perf difference, but this is a small patch that mostly touches test cases.

@lilyball
Copy link
Contributor

lilyball commented Jul 8, 2014

This may mostly touch test cases, but it sets a precedent and suggests more PRs should be submitted that do the same transformation elsewhere.

My recommendation is to avoid doing any of this work until we have a better idea about how to make "foo".to_string() become cheap again.

@cburgdorf
Copy link
Contributor Author

Ah, well, never mind then. I was referring to iron/iron#86 (comment) and thought it might be a patch small enough to get me started with some non-documentation patches. I'll find something different to hack on ;-)

@lilyball
Copy link
Contributor

lilyball commented Jul 8, 2014

@cburgdorf That's a reasonable reaction to that comment, TBH. It just happens to come at an inopportune time. For new code, certainly, use "foo".to_string(), that is the idiomatically correct approach. But there's no convincing win to modifying existing uses of String::from_str() at this time.

Thank you for your contribution, and I'm looking forward to whatever else you submit. I'm closing this PR at this time for the already-discussed reasons. Once "foo".to_string() becomes fast again, I would encourage you to consider re-submitting.

@lilyball lilyball closed this Jul 8, 2014
@cburgdorf
Copy link
Contributor Author

👍

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.

5 participants