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 for more types on Cow #50170

Merged
merged 6 commits into from
May 17, 2018
Merged

Conversation

burtonageo
Copy link
Contributor

This is basically #48191, except that it should be implemented in a way that doesn't break third party crates.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2018
@KodrAus
Copy link
Contributor

KodrAus commented Apr 25, 2018

Thanks @burtonageo! At first glance this looks good to me, there are a few more concrete implementations like From<Cow<'a, T>> for T::Owned that we could add here too:

  • From<Cow<'a, Path>> for PathBuf
  • From<Cow<'a, CStr>> for CString
  • From<Cow<'a, OsStr>> for OsString

These probably aren't strictly necessary, but the From<Cow<'a, [T]>> for Vec<T> was also only added for consistency with the String implementation.

@burtonageo
Copy link
Contributor Author

pathbuf_from_cow_path, cstring_from_cow_cstr and osstring_from_cow_osstr all added

@KodrAus
Copy link
Contributor

KodrAus commented May 1, 2018

Thanks @burtonageo! This looks good to me.

@rfcbot fcp merge

These conversions are insta-stable, but I don't think they're contentious. We now have a pattern with our From conversions for Cow between T and T::Owned:

impl<'a, T> From<Cow<'a, T>> for T::Owned
impl<'a, T> From<&'a T> for Cow<'a, T> 
impl<'a, T> From<T::Owned> for Cow<'a, T> 
impl<'a, T> From<&'a T::Owned> for Cow<'a, T>

This brings our set of From conversions to:

  impl<'a, T> From<Cow<'a, [T]>> for Vec<T>
  impl<'a, T> From<&'a [T]> for Cow<'a, [T]> 
  impl<'a, T> From<Vec<T>> for Cow<'a, [T]> 
+ impl<'a, T> From<&'a Vec<T>> for Cow<'a, [T]>

  impl<'a> From<Cow<'a, str>> for String
  impl<'a> From<&'a str> for Cow<'a, str>
  impl<'a> From<String> for Cow<'a, str>
+ impl<'a> From<&'a String> for Cow<'a, str>

+ impl<'a> From<Cow<'a, Path>> for PathBuf
  impl<'a> From<&'a Path> for Cow<'a, Path>
  impl<'a> From<PathBuf> for Cow<'a, Path>
+ impl<'a> From<&'a PathBuf> for Cow<'a, Path>

+ impl<'a> From<Cow<'a, CStr>> for CString
+ impl<'a> From<&'a CStr> for Cow<'a, CStr>
+ impl<'a> From<CString> for Cow<'a, CStr>
+ impl<'a> From<&'a CString> for Cow<'a, CStr>

+ impl<'a> From<Cow<'a, OsStr>> for OsString
+ impl<'a> From<&'a OsStr> for Cow<'a, OsStr>
+ impl<'a> From<OsString> for Cow<'a, OsStr>
+ impl<'a> From<&'a OsString> for Cow<'a, OsStr>

@KodrAus
Copy link
Contributor

KodrAus commented May 6, 2018

@rfcbot fcp merge

@KodrAus KodrAus added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 6, 2018
@KodrAus
Copy link
Contributor

KodrAus commented May 6, 2018

One more time...

@rfcbot fcp merge

@KodrAus
Copy link
Contributor

KodrAus commented May 6, 2018

@rfcbot we are no longer friends.

I could just r+ this but figured I'd use rfcbot since these impls won't have an unstable period.

cc @rust-lang/libs

@SimonSapin
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented May 6, 2018

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 6, 2018
@@ -2239,6 +2239,14 @@ impl<'a> From<String> for Cow<'a, str> {
}
}

#[stable(feature = "cow_from_string_ref", since = "1.27.0")]
Copy link
Member

Choose a reason for hiding this comment

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

These should all become 1.28.0

@burtonageo
Copy link
Contributor Author

@KodrAus Can this now enter final comment period?

@kennytm
Copy link
Member

kennytm commented May 11, 2018

@burtonageo

It needs one more vote from either @Kimundi, @aturon or @withoutboats.

@TimNN TimNN added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2018
@TimNN
Copy link
Contributor

TimNN commented May 15, 2018

Ping, @Kimundi, @aturon or @withoutboats: This PR has an open FCP request that needs your input.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 16, 2018
@rfcbot
Copy link

rfcbot commented May 16, 2018

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

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 16, 2018

📌 Commit 7c0f664 has been approved by alexcrichton

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 16, 2018
@kennytm kennytm added the relnotes Marks issues that should be documented in the release notes of the next release. label May 16, 2018
kennytm added a commit to kennytm/rust that referenced this pull request May 16, 2018
…chton

Implement From for more types on Cow

This is basically rust-lang#48191, except that it should be implemented in a way that doesn't break third party crates.
bors added a commit that referenced this pull request May 17, 2018
Rollup of 17 pull requests

Successful merges:

 - #50170 (Implement From for more types on Cow)
 - #50638 (Don't unconditionally set CLOEXEC twice on every fd we open on Linux)
 - #50656 (Fix `fn main() -> impl Trait` for non-`Termination` trait)
 - #50669 (rustdoc: deprecate `#![doc(passes, plugins, no_default_passes)]`)
 - #50726 (read2: Use inner function instead of closure)
 - #50728 (Fix rustdoc panic with `impl Trait` in type parameters)
 - #50736 (env: remove unwrap in examples in favor of try op)
 - #50740 (Remove LazyBTreeMap.)
 - #50752 (Add missing error codes in libsyntax-ext asm)
 - #50779 (Make mutable_noalias and arg_align_attributes be tracked)
 - #50787 (Fix run-make wasm tests)
 - #50788 (Fix an ICE when casting a nonexistent const)
 - #50789 (Ensure libraries built in stage0 have unique metadata)
 - #50793 (tidy: Add a check for empty UI test files)
 - #50797 (fix a typo in signed-integer::from_str_radix())
 - #50808 (Stabilize num::NonZeroU*)
 - #50809 (GitHub: Stop treating Cargo.lock as a generated file.)

Failed merges:
@bors bors merged commit 7c0f664 into rust-lang:master May 17, 2018
@burtonageo burtonageo deleted the more_cow_from branch May 17, 2018 04:56
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 26, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jun 28, 2018

This PR did still cause breakage: #51844

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants