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 `ToOwned::clone_into` (`toowned_clone_into`) #41263

Open
scottmcm opened this Issue Apr 13, 2017 · 11 comments

Comments

Projects
None yet
7 participants
@scottmcm
Member

scottmcm commented Apr 13, 2017

Feature added in PR #41009

Questions raised in the PR:

  • The directionality is weird. In clone_from and assignment, the data moves right-to-left, but this moves it left-to-right. And that means that autoref doesn't work well, usually forcing you to write &mut.

  • But fixing that would mean putting it somewhere else, since the Self in ToOwned is the wrong type for what this needs to be. And moving it while still being overridable and providing a default is hard.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 17, 2018

Contributor

In nightly for 11 months with no reported issues since. Let’s stabilize.

@rfcbot fcp merge

Contributor

SimonSapin commented Mar 17, 2018

In nightly for 11 months with no reported issues since. Let’s stabilize.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Mar 17, 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 commented Mar 17, 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

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Mar 19, 2018

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

rfcbot commented Mar 19, 2018

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

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Mar 20, 2018

Member

Hmm, I was bad at creating tracking issues a year ago 😅 (fixed)

This is sufficient, but still feels suboptimal for the reasons from #41009 (comment):

  • The directionality is weird. In clone_from and assignment, the data moves right-to-left, but this moves it left-to-right. And that means that autoref doesn't work well, usually forcing you to write &mut.

  • But fixing that would mean putting it somewhere else, since the Self in ToOwned is the wrong type for what this needs to be. And moving it while still being overridable and providing a default is hard.

Any good ideas?

Edit: Hmm, maybe once we flatten the facade (since Borrow is in core but ToOwned is in alloc) then Borrow could have fn owned_from(&mut self, other: &Borrowed) where Borrowed: ToOwned<Owned=Self> { *self = other.to_owned(); }?

Member

scottmcm commented Mar 20, 2018

Hmm, I was bad at creating tracking issues a year ago 😅 (fixed)

This is sufficient, but still feels suboptimal for the reasons from #41009 (comment):

  • The directionality is weird. In clone_from and assignment, the data moves right-to-left, but this moves it left-to-right. And that means that autoref doesn't work well, usually forcing you to write &mut.

  • But fixing that would mean putting it somewhere else, since the Self in ToOwned is the wrong type for what this needs to be. And moving it while still being overridable and providing a default is hard.

Any good ideas?

Edit: Hmm, maybe once we flatten the facade (since Borrow is in core but ToOwned is in alloc) then Borrow could have fn owned_from(&mut self, other: &Borrowed) where Borrowed: ToOwned<Owned=Self> { *self = other.to_owned(); }?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 20, 2018

Member

@rfcbot concern direction

formally registering @scottmcm's concern

Member

alexcrichton commented Mar 20, 2018

@rfcbot concern direction

formally registering @scottmcm's concern

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Mar 29, 2018

The final comment period is now complete.

rfcbot commented Mar 29, 2018

The final comment period is now complete.

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril May 24, 2018

Contributor

@alexcrichton rfcbot did not register the concern :) (the syntax you used is accepted nowadays however..)

Contributor

Centril commented May 24, 2018

@alexcrichton rfcbot did not register the concern :) (the syntax you used is accepted nowadays however..)

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Jul 18, 2018

Member

I feel bad seeing this in TWiR every week now. Anything I can do to help it get somewhere productive?

Member

scottmcm commented Jul 18, 2018

I feel bad seeing this in TWiR every week now. Anything I can do to help it get somewhere productive?

@kennytm kennytm added the I-nominated label Jul 18, 2018

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jul 18, 2018

Member

Nominating for discussion (how to address the concern raised in #41263 (comment)). ↑.

Member

kennytm commented Jul 18, 2018

Nominating for discussion (how to address the concern raised in #41263 (comment)). ↑.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Aug 1, 2018

Contributor

It looks like this still needs some API some design work, so we’re not actually ready to stabilize.

@rfcbot fcp cancel

This probably needs someone to champion a new API proposal.

Contributor

SimonSapin commented Aug 1, 2018

It looks like this still needs some API some design work, so we’re not actually ready to stabilize.

@rfcbot fcp cancel

This probably needs someone to champion a new API proposal.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Aug 1, 2018

@SimonSapin proposal cancelled.

rfcbot commented Aug 1, 2018

@SimonSapin proposal cancelled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment