-
Notifications
You must be signed in to change notification settings - Fork 14k
Deprecate MaybeOwned[Vector] in favor of Cow #19252
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
Conversation
There was a problem hiding this comment.
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.
|
I'm very excited to see this! I will review today or tomorrow. |
|
This PR makes me very happy 👍 I left a minor comment or two.
I'm fine with replacing 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. |
|
@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 |
Makes sense to me. |
|
OK, I've reviewed the new commits, and it all looks good to me. Thanks for doing this! |
|
@aturon Squashed, rebased and passed |
- 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
There was a problem hiding this comment.
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.
Fix syntax fixup producing invalid punctuation
IntoCowtrait, and put it in the preludeis_owned/is_borrowedmethods toCowCowString/CowVectype aliases (toCow<'_, String, str>/Cow<'_, Vec, [T]>respectively)Cowimplements:Show,Hash,[Partial]{Eq,Ord}impl BorrowFrom<Cow<'a, T, B>> for B[breaking-change]s:
IntoMaybeOwnedhas been removed from the preludeSendStris now an alias toCowString<'static>(it was aliased toMaybeOwned<'static>)LabelTextvariants now wrapCowStringinstead ofMaybeOwnedNodesandEdgesare now type aliases toCowVec(they were aliased toMaybeOwnedVec)Display::as_maybe_ownedhas been renamed toDisplay::as_cowand now returns aCowStringCowinstead ofMaybeOwned[Vector]:Replacer::reg_replacestr::from_utf8_lossyId::new,Id::name,LabelText::pre_escaped_contentTaskBuilder::namedr? @aturon