-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Extend Cell to work with non-Copy types #39287
Conversation
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
/// ``` | ||
#[stable(feature = "rust1", since = "1.0.0")] | ||
#[inline] | ||
pub const fn new(value: T) -> Cell<T> { |
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 think doing stable inside unstable makes the item stable again. At least that was the problem of bug #38860
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.
Yeah, I'm not sure what the right thing to do here is. There needs to be some way to construct a non-Copy
Cell
but perhaps it would be better to add a new function?
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 would advise moving the unstable
attribute downward and applying it to each of the new methods. That generally makes it easier to track stabilization later, anyway.
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'll do that. Do you think I should create a new function instead of moving new()
here or is this ok?
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 think new
should be modified, given that we're changing set
as well. But cc @rust-lang/libs (since this makes the change roughly insta-stable).
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.
Yeah I considered this as an implication of accepting the RFC, so immediately generalizing the behavior of new
and set
seems ok to me.
Should the documentation around line 18 en 182 also be updated? |
Part of the motivation and drawbacks sections of the RFC help to get a mental model of the new 'rules' around |
@pitdicker That's a great suggestion, thank you! |
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.
Looks great! Just a few minor points re: stability.
/// ``` | ||
#[stable(feature = "rust1", since = "1.0.0")] | ||
#[inline] | ||
pub const fn new(value: T) -> Cell<T> { |
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 would advise moving the unstable
attribute downward and applying it to each of the new methods. That generally makes it easier to track stabilization later, anyway.
src/libcore/cell.rs
Outdated
} | ||
} | ||
|
||
#[unstable(feature = "move_cell", issue = "39264")] |
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.
Same here -- please move the unstable
down to the method.
@aturon Fixed @pitdicker What do you think of these doc changes? |
e695e76
to
94cc522
Compare
@wesleywiser I would like to take a better look at this and the surrounding documentation. But I don't have much time today, maybe tomorrow. But thank you for taking the effort :). Don't let my reaction hold up this PR. |
@pitdicker Thanks for pointing that out! I don't mind updating that but I'd like to have somebody look at the doc changes I've already made and make sure they make sense before I try to do the same ones on the book. |
It makes sense to me for what that's worth. |
src/libcore/cell.rs
Outdated
//! mutability by moving values in and out of the `Cell<T>`. To use references instead of values, | ||
//! one must use the `RefCell<T>` type, acquiring a write lock before mutating. `Cell<T>` provides | ||
//! methods to retrieve and change the current interior value. For types that implement `Copy`, | ||
//! the `get` method retrieves the current interior value. For types that implement `Default`, |
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.
This is great, but you might consider using a bullet list for each of the "For types that implement" sentences, which would make this easier to read.
The docs look good to me as well. I left a minor style suggestion, but at this point, r=me. @wesleywiser, let me know if you want to make further changes; otherwise, we can send this on to bors. Thanks for the PR! |
Thanks @glaebhoerl and @aturon! I'd like to update the book and implement your suggestion before sending this to bors. |
94cc522
to
8b947a3
Compare
Ok, the book is updated and I implemented the style suggestion. If you have any more feedback, please let me know, otherwise I think this is good to go. |
Great, thank you! @bors: r+ |
📌 Commit 8b947a3 has been approved by |
Extend Cell to work with non-Copy types I'm not sure that I did this right but all of the tests pass. I also had to move the `new()` function so that `Cell`s with non-`Copy` `T`s could be created. That wasn't in the RFC but I assume it needed to be done?
☀️ Test successful - status-appveyor, status-travis |
let old = self.replace(val); | ||
drop(old); |
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.
@wesleywiser do we actually need old
here? Also why the explicit drop
? Mostly just trying to make sure I'm not missing some subtle detail :)
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.
This is a comment on a very old PR. :D But no, that drop is not necessary, it probably was just there for explicitness.
I'm not sure that I did this right but all of the tests pass.
I also had to move the
new()
function so thatCell
s with non-Copy
T
s could be created. That wasn't in the RFC but I assume it needed to be done?