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

Remove RcMut and Cell and introduce Mutable #9351

Closed
bluss opened this issue Sep 20, 2013 · 12 comments
Closed

Remove RcMut and Cell and introduce Mutable #9351

bluss opened this issue Sep 20, 2013 · 12 comments

Comments

@bluss
Copy link
Member

bluss commented Sep 20, 2013

Introduce Mutable<T> to implement a dynamic mutable slot, with dynamic borrow check, in once place. Obsolete Cell<T> and RcMut<T> (and what about @mut T? See #7140)

For Mutable<T> use an implementation with wrapped borrowed pointers to allow for both closured-bracketed and "roaming" borrowed pointers.

A working test is https://gist.github.com/anonymous/f007d9b907509c03c17f

Updated, WIP: https://github.com/blake2-ppc/rust/compare/mutable-t

@alexcrichton
Copy link
Member

This is interesting because not too long ago we had Mut<T> and removed it in favor of Cell... I haven't been following this much, though, and neither did I follow that, so don't mind me much.

@bluss
Copy link
Member Author

bluss commented Sep 20, 2013

Either way, I think we want a Cell that works more like this Mutable w.r.t borrowing.

@sfackler
Copy link
Member

Mut as implemented in that Gist isn't sufficient to replace Cell. Cell is currently used to do (at least) two distinct things: override mutability restrictions and override move restrictions. Mut will take care of the first one, but unless once fns are officially adopted into the language, we still need something that can do things like

let foo = Cell::new(10);
do spawn {
    let foo = foo.take();
    ....
}

Maybe Cell will stay around to do this? We could probably remove all methods except for take and put_back in that case.

@bluss
Copy link
Member Author

bluss commented Sep 20, 2013

Mutable<Option<T>> can do all the same things as Cell<T>

// Cell-like
impl<T> Mutable<Option<T>> {
    pub fn take(&self) -> Option<T> {
        let mut mptr = self.borrow_mut();
        mptr.get().take()
    }

    pub fn put_back(&self, x: T) {
        let mut mptr = self.borrow_mut();
        assert!(mptr.get().is_none());
        *mptr.get() = Some(x);
    }
}

Edited to add: The point is indeed to separate the factors. Mutable provides the dynamic mutable slot, Option provides the take / put_back, and Rc can be used to replace RcMut with Rc<Mutable<T>>

@sfackler
Copy link
Member

It seems a bit unwieldy for the relatively common task of moving things into closures:

let foo = Mut::new(Some(10));
do spawn {
    let foo = foo.borrow_mut();
    let foo = foo.get().take();
    ...
}

spawn isn't actually that great of an example since there's spawn_with, but there are plenty of other methods without a _with version.

@bluss
Copy link
Member Author

bluss commented Sep 20, 2013

if take is implemented directly on Mutable<Option<T>> like the sketch in my previous comment it would be this:

let foo = Mutable::new(Some(10));
do spawn {
    let foo = foo.take();
    ...
}

Edited 12 hours later: My turn to be wrong :-( it's really let foo = foo.take().unwrap()

@sfackler
Copy link
Member

Huh, I didn't know you could specialize impls like that.

In any case I like the idea of adding Mut.

@Thiez
Copy link
Contributor

Thiez commented Sep 20, 2013

For historical reference, the PR that removed Mut: #5678, the removal of which was part of removing mut fields. As I recall the only reason Cell wasn't removed was the problem with closures, and because it was used in Servo?
Some relevant IRC logs: https://botbot.me/mozilla/rust/msg/2818497/
I couldn't find the older one with a conversation I had with pcwalton (I think it was before [o__o] joined the rust irc channel and irclog.gr has a really crappy search interface.

@thestinger
Copy link
Contributor

@Thiez: @mut still exists and RcMut implements a similar mechanism, so the concept still exists and the proposal only involves factoring out the duplication + making it less coarse

@bluss
Copy link
Member Author

bluss commented Sep 21, 2013

I'm working on this at https://github.com/blake2-ppc/rust/compare/mutable-t

  • I'm gradually replacing Cell. The pattern Cell::new(x) with an immediate cell.take() is overwhelmingly common. Should Cell still be its own type for just that pattern?
  • The BorrowFlag should be smaller than the current 2 x uint..
  • Should Mut<T> have the cell like interface for Mut<Option<U>>? Indeed Cell seems simple enough for many of its uses, and Mut might only complement the cases where you need to borrow a held value.
  • Even empty destructors like the one of NonCopyable will insert a destructor function call, and this should be the only overhead of Mut over Cell for the simplest use cases.

@bluss
Copy link
Member Author

bluss commented Sep 23, 2013

Posted RFC pull request with explanation as #9429 ; I prefer to have a real implementation as basis for discussion.

@thestinger
Copy link
Contributor

These are both gone.

flip1995 pushed a commit to flip1995/rust that referenced this issue Sep 9, 2022
Fixes rust-lang#9351.

Note that this commit reworks that fix for rust-lang#9317. The change
is to check that the type implements `AsRef<str>` before regarding
`to_string` as an equivalent of `to_owned`. This was suggested
by Jarcho in the rust-lang#9317 issue comments.

The benefit of this is that it moves some complexity out of
`check_other_call_arg` and simplifies the module as a whole.
flip1995 pushed a commit to flip1995/rust that referenced this issue Sep 9, 2022
Fix `unnecessary_to_owned` false positive

Fixes rust-lang#9351.

Note that this commit reworks that fix for rust-lang#9317. The change
is to check that the type implements `AsRef<str>` before regarding
`to_string` as an equivalent of `to_owned`. This was suggested
by Jarcho in the rust-lang#9317 issue comments.

The benefit of this is that it moves some complexity out of
`check_other_call_arg` and simplifies the module as a whole.

changelog: FP: [`unnecessary_to_owned`]: No longer lints, if type change would cause errors in the caller function
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

No branches or pull requests

5 participants