Skip to content

Str -> String #15075

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 2 commits into from
Closed

Str -> String #15075

wants to merge 2 commits into from

Conversation

richo
Copy link
Contributor

@richo richo commented Jun 21, 2014

Opening a PR because my build environment is wedged on this machine.

@richo
Copy link
Contributor Author

richo commented Jun 21, 2014

Updates #15046

@Sawyer47
Copy link
Contributor

What about StrAllocating::to_string?

@alexcrichton
Copy link
Member

Note that #15046 was just a suggestion, no official decision has been made. It also looks like there's some travis errors.

I'm saddened that our string conversions keep getting more and more wordier, but I agree that this is more consistent with today's terminology.

@schmee
Copy link
Contributor

schmee commented Jun 21, 2014

@alexcrichton How about to_s? Shorter, and immune to whatever renaming might occur (if it was to_s the name could have stayed the same thought the ~str -> StrBuf -> String conversions).

@brson
Copy link
Contributor

brson commented Jun 27, 2014

This approach has been generally agreed on and I think we can move forward. It is more verbose, following recent trends, but if it ends up being bad we will circle back around and think about ergonomics (we need some wholistic work on string ergonomics anyway). @richo can you rebase?

@richo
Copy link
Contributor Author

richo commented Jun 27, 2014

Sure, I can do it tonight.

@brson
Copy link
Contributor

brson commented Jul 1, 2014

I've started on a rebase of this. It's complicated by StrAllocating now having its own to_string method. With both StrAllocating and ToString in the prelude, to_string becomes impossible to call on &str, so ... some thought is needed here.

I'm going to try to just temporarily rename StrAllocating::to_string and see what the fallout is.

@richo
Copy link
Contributor Author

richo commented Jul 3, 2014

I'm just finding time to do this now. What's your branch looking like @brson? Should I do it or leave it for you?

@brson
Copy link
Contributor

brson commented Jul 4, 2014

@richo I've made good progress. I'd like to finish it if you don't mind. Sorry for stepping on your toes.

@richo
Copy link
Contributor Author

richo commented Jul 4, 2014

No it's not at all a problem. I'm just interested in seeing issues get fixed :)

Just wanted to make sure we weren't duplicating effort, which would suck for all of us.

@brson
Copy link
Contributor

brson commented Jul 7, 2014

Rebased in #15493.

@brson brson closed this Jul 7, 2014
bors added a commit that referenced this pull request Jul 8, 2014
This updates #15075.

Rename `ToStr::to_str` to `ToString::to_string`. The naive renaming ends up with two `to_string` functions defined on strings in the prelude (the other defined via `collections::str::StrAllocating`). To remedy this I removed `StrAllocating::to_string`, making all conversions from `&str` to `String` go through `Show`. This has a measurable impact on the speed of this conversion, but the sense I get from others is that it's best to go ahead and unify `to_string` and address performance for all `to_string` conversions in `core::fmt`. `String::from_str(...)` still works as a manual fast-path.

Note that the patch was done with a script, and ended up renaming a number of other `*_to_str` functions, particularly inside of rustc. All the ones I saw looked correct, and I didn't notice any additional API breakage.

Closes #15046.
cdwort added a commit to cdwort/rust_for_rubyists that referenced this pull request Aug 13, 2014
lnicola pushed a commit to lnicola/rust that referenced this pull request Jun 19, 2023
fix: Add binding definition for for-expr iterator desugared binding
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024
This updates rust-lang/rust#15075.

Rename `ToStr::to_str` to `ToString::to_string`. The naive renaming ends up with two `to_string` functions defined on strings in the prelude (the other defined via `collections::str::StrAllocating`). To remedy this I removed `StrAllocating::to_string`, making all conversions from `&str` to `String` go through `Show`. This has a measurable impact on the speed of this conversion, but the sense I get from others is that it's best to go ahead and unify `to_string` and address performance for all `to_string` conversions in `core::fmt`. `String::from_str(...)` still works as a manual fast-path.

Note that the patch was done with a script, and ended up renaming a number of other `*_to_str` functions, particularly inside of rustc. All the ones I saw looked correct, and I didn't notice any additional API breakage.

Closes #15046.
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