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

Implement From<&mut str> for String #69661

Merged
merged 3 commits into from Mar 15, 2020
Merged

Conversation

@lopopolo
Copy link
Contributor

lopopolo commented Mar 3, 2020

I ran into this missing impl when trying to do String::from on the result returned from this API in the uuid crate:

https://docs.rs/uuid/0.8.1/uuid/adapter/struct.Hyphenated.html#method.encode_lower

I wasn't sure what to put in the stability annotation. I'd appreciate some help with that :)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 3, 2020

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @shepmaster (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Aaron1011

This comment has been minimized.

Copy link
Member

Aaron1011 commented Mar 4, 2020

I'm not sure if it's a good idea to hide an allocation behind a From impl. Is there a situtation where you can't just call str.to_owned() directly?

@LeSeulArtichaut

This comment has been minimized.

Copy link
Contributor

LeSeulArtichaut commented Mar 4, 2020

@Aaron1011 There already are some From implementations that allocate. Even From<&str> for String:

rust/src/liballoc/string.rs

Lines 2221 to 2226 in 2b0cfa5

impl From<&str> for String {
#[inline]
fn from(s: &str) -> String {
s.to_owned()
}
}

@lopopolo As Aaron1011 pointed out above, to avoid your current problem, you could call .to_string() or to_owned() on the &mut str instead of using String::from(), which does just the same thing.

@lopopolo

This comment has been minimized.

Copy link
Contributor Author

lopopolo commented Mar 4, 2020

I know that to_owned does the same thing as the From impl I added in this PR (I authored the PR after all). However I already use String::from everywhere in my codebase to turn &str into String. It seems arbitrary that I cannot do the same for &mut str

@LeSeulArtichaut

This comment has been minimized.

Copy link
Contributor

LeSeulArtichaut commented Mar 4, 2020

By the way, does any conventions exist on which one to preferably use?

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Mar 5, 2020

In general, this change makes sense to me because we already have the equivalent for &str. However, it's modifying public API, so I'm going to kick it to the libs team...

r? @cramertj

@rust-highfive rust-highfive assigned cramertj and unassigned shepmaster Mar 5, 2020
@Aaron1011

This comment has been minimized.

Copy link
Member

Aaron1011 commented Mar 5, 2020

I didn't realize that we already had impl From<&str> for String. I withdraw my previous concern.

@tspiteri

This comment has been minimized.

Copy link
Contributor

tspiteri commented Mar 5, 2020

While From<&str> for String is quite useful for, for example, String::from("hello"), I'm not quite sure that means there should be From<&mut str>; you could always use String::from(&*str_mut).

Somehow equivalently, lots of arithmetic operations are implemented for &i32 etc. such that you can add &a + &b, but you cannot add &mut a + &mut b.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Mar 6, 2020

However, it's modifying public API, so I'm going to kick it to the libs team...
r? @cramertj

Forwarding this to a libs team member ;)

r? @sfackler

@Centril Centril modified the milestones: 1.43, 1.44 Mar 10, 2020
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Mar 10, 2020

Seems like highfive didn't listen, r? @LukasKalbertodt

Copy link
Member

LukasKalbertodt left a comment

While I'm not particularly fond of adding impls like that for mutable references, I think this one is fine to add. Not only is From<&str> for String implemented, but more importantly, there is also this impl:

impl<'_, T: CloneFrom<&'_ mut [T]> for Vec<T>

So I'd be fine with starting an FCP to merge this after my line comment is dealt with and you added some docs (as @LeSeulArtichaut suggested).

src/liballoc/string.rs Outdated Show resolved Hide resolved
@lopopolo

This comment has been minimized.

Copy link
Contributor Author

lopopolo commented Mar 11, 2020

@LukasKalbertodt thanks for the review. I've pushed some additional commits that address the feedback.

Copy link
Member

LukasKalbertodt left a comment

Great, thanks!

@LukasKalbertodt

This comment has been minimized.

Copy link
Member

LukasKalbertodt commented Mar 11, 2020

As I said, I am in favor of this change. But I can't start FCP merges, so I have to reassign:
r? @Amanieu

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Mar 12, 2020

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 12, 2020

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 15, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Mar 15, 2020

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 15, 2020

📌 Commit 533784d has been approved by sfackler

bors added a commit that referenced this pull request Mar 15, 2020
Rollup of 8 pull requests

Successful merges:

 - #69528 (Add undo_leak to reset RefCell borrow state)
 - #69589 (ast: `Mac`/`Macro` -> `MacCall`)
 - #69661 (Implement From<&mut str> for String)
 - #69988 (rustc_metadata: Remove `rmeta::MacroDef`)
 - #70006 (resolve: Fix two issues in fresh binding disambiguation)
 - #70011 (def_collector: Fully visit async functions)
 - #70013 (Return feature gate as a `Symbol` )
 - #70018 (Fix "since" field for `Once::is_complete`'s `#[stable]` attribute)

Failed merges:

r? @ghost
@bors bors merged commit cc16232 into rust-lang:master Mar 15, 2020
4 checks passed
4 checks passed
pr Build #20200311.3 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.