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

Add example for Cow #53113

Merged
merged 3 commits into from
Aug 31, 2018
Merged

Add example for Cow #53113

merged 3 commits into from
Aug 31, 2018

Conversation

kpp
Copy link
Contributor

@kpp kpp commented Aug 6, 2018

Add one more example that shows how to keep Cow in a struct.

Link to playground: https://play.rust-lang.org/?gist=a9256bdd034b44bc3cdd0044bbcdbb7c&version=stable&mode=debug&edition=2015

Users ask this question in ruRust chat time to time and it is not obvious to add ToOwned<Owned=Target> to requirements of generic params.

@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 @withoutboats (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 Aug 6, 2018
@TimNN
Copy link
Contributor

TimNN commented Aug 14, 2018

Ping from triage @withoutboats / @rust-lang/docs: This PR requires your review.

@@ -141,6 +141,40 @@ impl<T> ToOwned for T
/// let mut input = Cow::from(vec![-1, 0, 1]);
/// abs_all(&mut input);
/// ```
///
Copy link
Member

Choose a reason for hiding this comment

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

Please add an "introduction" sentence explaining what does this new example (otherwise it's strange to just have two examples following each other).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// _ => panic!("expect owned data"),
/// }
/// ```

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this empty line.

@frewsxcv
Copy link
Member

Thanks for the docs addition @kpp! You mentioned that users in the ruRust chat have been asking how to do this. Do you think the main reason users are struggling with this because of the [X]: ToOwned<Owned=Vec<X> syntax? If so, do you think it would be sufficient to just have a something like this?

/// If you want to have a structure with a generic type that implements `ToOwned`:
///
/// ```rust
/// struct Items<'a, X: 'a>
///     where [X]: ToOwned<Owned=Vec<X>>
/// {
///     values: Cow<'a, [X]>,
/// }
/// ```

@kpp
Copy link
Contributor Author

kpp commented Aug 15, 2018

struggling with this because of the [X]: ToOwned<Owned=Vec<X>> syntax

It is hard to tell. Maybe because in this example you don't need to specify [i32]: ToOwned<Owned=Vec<i32>>.

Here is how users try to implement it (forgetting to specify ToOwned<Owned=Vec<X>>):

https://play.rust-lang.org/?gist=46d00be1797064d67deeee75383e7a7e&version=stable&mode=debug&edition=2015

struct Items<'a, X> where [X]: 'a {
    values: Cow<'a, [X]>,
}

They got this error:

error[E0277]: the trait bound `[X]: std::borrow::ToOwned` is not satisfied
 --> src/main.rs:5:5
  |
5 |     values: Cow<'a, [X]>,
  |     ^^^^^^^^^^^^^^^^^^^^ the trait `std::borrow::ToOwned` is not implemented for `[X]`
  |
  = help: consider adding a `where [X]: std::borrow::ToOwned` bound
  = note: required by `std::borrow::Cow`

They add [X]: std::borrow::ToOwned (without Owned=Vec<X> as the compiler did not mention it):

struct Items<'a, X> where [X]: 'a + ToOwned {
    values: Cow<'a, [X]>,
}

And they get sad errors like:

error[E0599]: no method named `push` found for type `&mut <[X] as std::borrow::ToOwned>::Owned` in the current scope
  --> src/main.rs:10:30
   |
10 |         self.values.to_mut().push(x);
   |                              ^^^^

If so, do you think it would be sufficient to just have a something like this?

Probably yes. Or shall we change the compiler warning?

@@ -142,6 +142,7 @@ impl<T> ToOwned for T
/// abs_all(&mut input);
/// ```
///
/// Another example showing how to keep `Cow` in a struct:
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

Now please add an "empty" line before the example. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@GuillaumeGomez
Copy link
Member

All good for me.

@pietroalbini
Copy link
Member

Ping from triage @withoutboats! This PR needs your review.
Otherwise, if you're OK with it, @GuillaumeGomez can you approve the changes?

@GuillaumeGomez
Copy link
Member

I'll go ahead and r+ it then. Thanks @kpp!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Aug 27, 2018

📌 Commit 5bfb785 has been approved by GuillaumeGomez

@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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Aug 28, 2018
…omez

Add example for Cow

Add one more example that shows how to keep `Cow` in a struct.

Link to playground: https://play.rust-lang.org/?gist=a9256bdd034b44bc3cdd0044bbcdbb7c&version=stable&mode=debug&edition=2015

Users ask this question in [ruRust](https://gitter.im/ruRust/general) chat time to time and it is not obvious to add `ToOwned<Owned=Target>` to requirements of generic params.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 28, 2018
…omez

Add example for Cow

Add one more example that shows how to keep `Cow` in a struct.

Link to playground: https://play.rust-lang.org/?gist=a9256bdd034b44bc3cdd0044bbcdbb7c&version=stable&mode=debug&edition=2015

Users ask this question in [ruRust](https://gitter.im/ruRust/general) chat time to time and it is not obvious to add `ToOwned<Owned=Target>` to requirements of generic params.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 29, 2018
…omez

Add example for Cow

Add one more example that shows how to keep `Cow` in a struct.

Link to playground: https://play.rust-lang.org/?gist=a9256bdd034b44bc3cdd0044bbcdbb7c&version=stable&mode=debug&edition=2015

Users ask this question in [ruRust](https://gitter.im/ruRust/general) chat time to time and it is not obvious to add `ToOwned<Owned=Target>` to requirements of generic params.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 29, 2018
…omez

Add example for Cow

Add one more example that shows how to keep `Cow` in a struct.

Link to playground: https://play.rust-lang.org/?gist=a9256bdd034b44bc3cdd0044bbcdbb7c&version=stable&mode=debug&edition=2015

Users ask this question in [ruRust](https://gitter.im/ruRust/general) chat time to time and it is not obvious to add `ToOwned<Owned=Target>` to requirements of generic params.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 30, 2018
…omez

Add example for Cow

Add one more example that shows how to keep `Cow` in a struct.

Link to playground: https://play.rust-lang.org/?gist=a9256bdd034b44bc3cdd0044bbcdbb7c&version=stable&mode=debug&edition=2015

Users ask this question in [ruRust](https://gitter.im/ruRust/general) chat time to time and it is not obvious to add `ToOwned<Owned=Target>` to requirements of generic params.
bors added a commit that referenced this pull request Aug 31, 2018
Rollup of 20 pull requests

Successful merges:

 - #51760 (Add another PartialEq example)
 - #53113 (Add example for Cow)
 - #53129 (remove `let x = baz` which was obscuring the real error)
 - #53389 (document effect of join on memory ordering)
 - #53472 (Use FxHash{Map,Set} instead of the default Hash{Map,Set} everywhere in rustc.)
 - #53476 (Add partialeq implementation for TryFromIntError type)
 - #53513 (Force-inline `shallow_resolve` at its hottest call site.)
 - #53655 (set applicability)
 - #53702 (Fix stabilisation version for macro_vis_matcher.)
 - #53727 (Do not suggest dereferencing in macro)
 - #53732 (save-analysis: Differentiate foreign functions and statics.)
 - #53740 (add llvm-readobj to llvm-tools-preview)
 - #53743 (fix a typo: taget_env -> target_env)
 - #53747 (Rustdoc fixes)
 - #53753 (expand keep-stage --help text)
 - #53756 (Fix typo in comment)
 - #53768 (move file-extension based .gitignore down to src/)
 - #53785 (Fix a comment in src/libcore/slice/mod.rs)
 - #53786 (Replace usages of 'bad_style' with 'nonstandard_style'.)
 - #53806 (Fix UI issues on Implementations on Foreign types)

Failed merges:

r? @ghost
@bors bors merged commit 5bfb785 into rust-lang:master Aug 31, 2018
@kpp kpp deleted the more_docs_for_cow branch August 31, 2018 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants