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 RefCell::{replace, swap} #43570

Open
alexcrichton opened this Issue Jul 31, 2017 · 15 comments

Comments

Projects
None yet
9 participants
@alexcrichton
Member

alexcrichton commented Jul 31, 2017

Tracking issue for rust-lang/rfcs#2057

  • Implement the functionality
  • Add documentation
  • Stabilize!
@notriddle

This comment has been minimized.

Contributor

notriddle commented Jul 31, 2017

Can I implement this? (I understand you'll want to give it the C-assigned label, to make sure nobody else works on it)

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Jul 31, 2017

Certainly!

bors added a commit that referenced this issue Aug 14, 2017

Auto merge of #43574 - notriddle:master, r=sfackler
Implement `RefCell::replace` and `RefCell::swap`

Tracking issue: #43570
@notriddle

This comment has been minimized.

Contributor

notriddle commented Aug 15, 2017

  • Implemented the functionality
  • What documentation is needed beyond the API docs?
  • I assume that once 1.20 is released, we'll be able to talk about marking it stable.
@Havvy

This comment has been minimized.

Contributor

Havvy commented Nov 7, 2017

The only docs needed for this is the API docs.

I've also written PR #45819 to add a replace_with variant.

@gbip

This comment has been minimized.

gbip commented Dec 5, 2017

Is there any plan to make this stable ? I can't figure the syntax to move a value inside a RefCell to replace the old one.
Thanks !

kennytm added a commit to kennytm/rust that referenced this issue Dec 20, 2017

Rollup merge of rust-lang#46517 - notriddle:patch-2, r=BurntSushi
Stablize RefCell::{replace, swap}

RefCell::replace_with is not stablized in this PR, since it wasn't part of the RFC.

CC rust-lang#43570
@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Mar 1, 2018

Stabilized in #46517

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 17, 2018

replace_with was not stabilized together with replace and swap in #46517 because it wasn’t part of rust-lang/rfcs#2057.

Let’s stabilize it now, perhaps with the “corresponds to std::mem::replace” bit of doc removed per #45819 (comment)

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

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:

Concerns:

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.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Mar 19, 2018

@rfcbot concern footgun

I'd personally be somewhat concerned about the replace_with API in terms of the intended use case of RefCell. Used in the case where mutability doesn't cut it usage of RefCell often implies lots of aliasing all over the place which is often difficult to determine statically. As a result correct progress mostly minimize the amount of time a RefCell is borrowed or otherwise used for. I'd be worried that allowing an arbitrary closure in replace_with is hiding the fact that you're borrowing the RefCell for the duration of the entire closure. It may be clearer to force authors to explicitly take a borrow and map it to see where the borrow duration runs?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Mar 19, 2018

I'm also under the impression that this isn't a massively used method in the sense that the ergonomic win here isn't necessary critical, but I could be wrong!

@Havvy

This comment has been minimized.

Contributor

Havvy commented Mar 19, 2018

I sent the PR for it because I wrote the function freely in my own code where I wanted to recurse through a path in a tree without passing a Vec of RefCell borrows but rather keep the borrow information in the stack.

Putting a warning that the RefCell is borrowed for the duration of the closure into the documentation would help.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented May 17, 2018

@SimonSapin do you have thoughts on the concern I listed above?

I'm not personally a huge fan of a warning in the sense that I don't think it absolves us of the footgun of the API, but if there's a strong use case for adding it then the warning is likely enough. I do not currently see, however, the strong use case for adding this

@Phlosioneer

This comment has been minimized.

Contributor

Phlosioneer commented Sep 23, 2018

This has been merged. Tracking issue can be closed.

@Havvy

This comment has been minimized.

Contributor

Havvy commented Sep 23, 2018

RefCell::replace_with has not been stabilized yet.

@Phlosioneer

This comment has been minimized.

Contributor

Phlosioneer commented Sep 23, 2018

Ah. I thought that would be a different issue, because it's not in the title and not in the RFC. Sorry.

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