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

Tracking issue for Cow::is_borrowed and Cow::is_owned #65143

Open
clarfonthey opened this issue Oct 5, 2019 · 12 comments
Open

Tracking issue for Cow::is_borrowed and Cow::is_owned #65143

clarfonthey opened this issue Oct 5, 2019 · 12 comments
Labels
B-unstable C-tracking-issue Libs-Tracked T-libs-api

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Oct 5, 2019

I just tried to use these in my own code and was kind of shocked they didn't exist.

Justification: this seems like a common Rust pattern. We have is_some and is_none for Option, is_ok and is_err for Result, etc., so, it seems pretty fair to have is_borrowed and is_owned for Cow.

Having as_borrowed and as_owned wouldn't really make much sense, as a simple & and &mut/to_mut cover those use cases. But, these check functions are pretty useful on their own.

@jonas-schievink jonas-schievink added C-feature-request T-libs-api C-tracking-issue B-unstable and removed C-feature-request labels Oct 5, 2019
@clarfonthey clarfonthey changed the title Cow::is_borrowed and Cow::is_owned Tracking issue: Cow::is_borrowed and Cow::is_owned Oct 5, 2019
@petertodd
Copy link
Contributor

petertodd commented Oct 12, 2019

I'm no expert on how backwards compatibility is handled. But Cow implements Deref, which means these new methods wouldn't be backwards compatible as they'd conflict with methods of the same name on the inner object. To fix this you'll have to change them to take cow: &Self, similar to what most of the methods on Box/Rc etc. take.

@SimonSapin SimonSapin changed the title Tracking issue: Cow::is_borrowed and Cow::is_owned Feature request: Cow::is_borrowed and Cow::is_owned Oct 18, 2019
@SimonSapin SimonSapin added C-feature-request and removed B-unstable C-tracking-issue labels Oct 18, 2019
@SimonSapin
Copy link
Contributor

SimonSapin commented Oct 18, 2019

Procedural: I’ve edited the title and labels to reflect that this is not implemented.

My opinion: I’m not sure that these method carry their weight, since if let Cow::Borrowed(_) = foo or matches!(foo, Cow::Borrowed(_)) can be used. Neither of these existed when Option::is_some was added, and Option is much more common than Cow.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Oct 22, 2019

@petertodd That's fair; that said, though, I think that having methods named is_borrowed or is_owned on a type which implements ToOwned would be extremely unusual, because to_owned would not affect said methods.

@SimonSapin I agree that Option is much more common than Cow, but I feel that these are still useful enough to carry their own weight. This is partially because I was writing code and was honestly surprised they didn't exist, so, I'm already pre-biased to assume that they should exist.

Mostly, I figured that adding these methods would be worthwhile because they're unlikely to clash with existing ones, and can incubate in nightly for a while to see if they do carry their weight. If they don't, they can be removed.

Centril added a commit to Centril/rust that referenced this issue Oct 23, 2019
Add Cow::is_borrowed and Cow::is_owned

Implements rust-lang#65143.
@clarfonthey
Copy link
Contributor Author

clarfonthey commented Oct 23, 2019

It appears that this is now merged.

Centril added a commit to Centril/rust that referenced this issue Oct 23, 2019
Add Cow::is_borrowed and Cow::is_owned

Implements rust-lang#65143.
@clarfonthey
Copy link
Contributor Author

clarfonthey commented Oct 27, 2019

@SimonSapin it would make sense to revert the changes to the title and tags, or discuss here whether the change should be reverted.

@SimonSapin SimonSapin changed the title Feature request: Cow::is_borrowed and Cow::is_owned Tracking issue for Cow::is_borrowed and Cow::is_owned Oct 27, 2019
@SimonSapin SimonSapin added B-unstable C-tracking-issue and removed C-feature-request labels Oct 27, 2019
@SimonSapin
Copy link
Contributor

SimonSapin commented Oct 27, 2019

Done.

@SimonSapin
Copy link
Contributor

SimonSapin commented Oct 27, 2019

Sorry for the somewhat-pointless churn, I’d gotten here from https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3AC-tracking-issue+label%3AT-libs and hadn’t realized there was an implementation PR already.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Oct 27, 2019

All good! Thanks for updating things :)

@KodrAus KodrAus added the Libs-Tracked label Jul 30, 2020
CDirkx pushed a commit to CDirkx/rust that referenced this issue Aug 31, 2020
Constify the following methods of `alloc::borrow::Cow`:
 - `is_borrowed`
 - `is_owned`

These methods are still unstable under `cow_is_borrowed`.
Possible because of rust-lang#49146 (Allow if and match in constants).

Tracking issue: rust-lang#65143
tmandry added a commit to tmandry/rust that referenced this issue Sep 1, 2020
…morse

Make `cow_is_borrowed` methods const

Constify the following methods of `alloc::borrow::Cow`:
 - `is_borrowed`
 - `is_owned`

Analogous to the const methods `is_some` and `is_none` for Option, and `is_ok` and `is_err` for Result.

These methods are still unstable under `cow_is_borrowed`.
Possible because of rust-lang#49146 (Allow if and match in constants).

Tracking issue: rust-lang#65143
@zbraniecki
Copy link

zbraniecki commented Mar 13, 2021

Is anything blocking this from being stabilized?

@fosskers
Copy link

fosskers commented Sep 30, 2021

Six month check-in: I don't see any issues involving these on the Issue Tracker. Can we move to the next stage?

@antmelnyk
Copy link

antmelnyk commented Jun 7, 2022

Quite sad it's not stabilized yet. Are there any issues? 🥲 I find the method would be pretty convenient when cloning of the content under Cow is bad and has to be double-checked for being Owned in critical places.

Example:

if my_expensive_thing.is_borrowed() {
  return Err(MyError::AttemptToCloneExpensiveThing);
} else {
  do_something_heavy(my_expensive_thing.to_mut());
}

@SimonSapin
Copy link
Contributor

SimonSapin commented Jun 7, 2022

At this point I feel this should be fine to stabilize, but until that happens you can match a pattern: #65143 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable C-tracking-issue Libs-Tracked T-libs-api
Projects
None yet
Development

No branches or pull requests

8 participants