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

Deprecate MaybeOwned[Vector] in favor of Cow #19252

Merged
merged 1 commit into from
Nov 26, 2014
Merged

Conversation

japaric
Copy link
Member

@japaric japaric commented Nov 23, 2014

  • Add IntoCow trait, and put it in the prelude
  • Add is_owned/is_borrowed methods to Cow
  • Add CowString/CowVec type aliases (to Cow<'_, String, str>/Cow<'_, Vec, [T]> respectively)
  • Cow implements: Show, Hash, [Partial]{Eq,Ord}
  • impl BorrowFrom<Cow<'a, T, B>> for B

[breaking-change]s:

  • IntoMaybeOwned has been removed from the prelude
  • libcollections: SendStr is now an alias to CowString<'static> (it was aliased to MaybeOwned<'static>)
  • libgraphviz:
    • LabelText variants now wrap CowString instead of MaybeOwned
    • Nodes and Edges are now type aliases to CowVec (they were aliased to MaybeOwnedVec)
  • libstd/path: Display::as_maybe_owned has been renamed to Display::as_cow and now returns a CowString
  • These functions now accept/return Cow instead of MaybeOwned[Vector]:
    • libregex: Replacer::reg_replace
    • libcollections: str::from_utf8_lossy
    • libgraphviz: Id::new, Id::name, LabelText::pre_escaped_content
    • libstd: TaskBuilder::named

r? @aturon

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@@ -512,7 +513,7 @@ pub trait GraphWalk<'a, N, E> {

/// Renders directed graph `g` into the writer `w` in DOT syntax.
/// (Main entry point for the library.)
pub fn render<'a, N:'a, E:'a, G:Labeller<'a,N,E>+GraphWalk<'a,N,E>, W:Writer>(
pub fn render<'a, N:Clone+'a, E:Clone+'a, G:Labeller<'a,N,E>+GraphWalk<'a,N,E>, W:Writer>(
Copy link
Member Author

Choose a reason for hiding this comment

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

I was forced to add the Clone bound here because the T in CowVec<T> needs to be cloneable. Not sure how bad this could be for downstream code.

@aturon
Copy link
Member

aturon commented Nov 23, 2014

I'm very excited to see this! I will review today or tomorrow.

@aturon
Copy link
Member

aturon commented Nov 24, 2014

This PR makes me very happy 👍 I left a minor comment or two.

Other things that we may want to do:

  • Add IntoCow to the prelude (since IntoMaybeOwned is in the prelude)
  • Add is_owned/is_borrowed inherent methods to Cow (MaybeOwned has these methods)

I'm fine with replacing IntoMaybeOwned with IntoCow in the prelude for now; we have a full prelude audit upcoming in any case.

I'm also fine adding the suggested predicates. In the long run, it's not clear to me whether we should expose the variants publicly as we do now, or newtype the representation and expose constructors/accessors. But this sounds fine in either case.

@japaric japaric changed the title [WIP] Deprecate MaybeOwned[Vector] in favor of Cow Deprecate MaybeOwned[Vector] in favor of Cow Nov 25, 2014
@japaric
Copy link
Member Author

japaric commented Nov 25, 2014

@aturon This is ready to go. r? (I can squash after you confirm everything is OK)

One extra thing I had to do that I didn't realized before, was impl BorrowFrom<Cow<'a, T, B>> for B. With this HashMap::get can be used on HashMaps with SendStr/Cow keys. This was necessary to get the HashMap<SendStr, _>::get run-pass tests to pass.

@aturon
Copy link
Member

aturon commented Nov 25, 2014

One extra thing I had to do that I didn't realized before, was impl BorrowFrom<Cow<'a, T, B>> for B. With this HashMap::get can be used on HashMaps with SendStr/Cow keys. This was necessary to get the HashMap<SendStr, _>::get run-pass tests to pass.

Makes sense to me.

@aturon
Copy link
Member

aturon commented Nov 25, 2014

OK, I've reviewed the new commits, and it all looks good to me. Thanks for doing this!

@japaric
Copy link
Member Author

japaric commented Nov 25, 2014

@aturon Squashed, rebased and passed make check after the rebase. r?

bors added a commit that referenced this pull request Nov 26, 2014
- Add `IntoCow` trait, and put it in the prelude
- Add `is_owned`/`is_borrowed` methods to `Cow`
- Add `CowString`/`CowVec` type aliases (to `Cow<'_, String, str>`/`Cow<'_, Vec, [T]>` respectively)
- `Cow` implements: `Show`, `Hash`, `[Partial]{Eq,Ord}`
- `impl BorrowFrom<Cow<'a, T, B>> for B`

[breaking-change]s:

- `IntoMaybeOwned` has been removed from the prelude
- libcollections: `SendStr` is now an alias to `CowString<'static>` (it was aliased to `MaybeOwned<'static>`)
- libgraphviz:
  - `LabelText` variants now wrap `CowString` instead of `MaybeOwned`
  - `Nodes` and `Edges` are now type aliases to `CowVec` (they were aliased to `MaybeOwnedVec`)
- libstd/path: `Display::as_maybe_owned` has been renamed to `Display::as_cow` and now returns a `CowString`
- These functions now accept/return `Cow` instead of `MaybeOwned[Vector]`:
  - libregex: `Replacer::reg_replace`
  - libcollections: `str::from_utf8_lossy`
  - libgraphviz: `Id::new`, `Id::name`, `LabelText::pre_escaped_content`
  - libstd: `TaskBuilder::named`

r? @aturon
@bors bors closed this Nov 26, 2014
@bors bors merged commit 3293ab1 into rust-lang:master Nov 26, 2014
}

/// Trait for moving into a `Cow`
pub trait IntoCow<'a, T, Sized? B> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be much easier to understand how the trait works if these type variables weren't single letters.

@japaric japaric deleted the cow branch December 16, 2014 02:10
lnicola pushed a commit to lnicola/rust that referenced this pull request Mar 10, 2025
Fix syntax fixup producing invalid punctuation
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