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

Implement cell::clone_ref #14189

Closed
wants to merge 1 commit into from
Closed

Conversation

kmcallister
Copy link
Contributor

Per discussion with @alexcrichton, this is a free function.

@@ -186,6 +186,24 @@ impl<'b, T> Deref<T> for Ref<'b, T> {
}
}

/// Copy a `Ref`. The `RefCell` is already immutably borrowed,
/// so this cannot fail.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prevailing convention is one space after the period.

Also, did you intend for the second sentence to be part of the method summary? I would expect that to be in the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; fixed.

Per discussion with @alexcrichton, this is a free function.
/// A `Clone` implementation would interfere with the widespread
/// use of `r.borrow().clone()` to clone the contents of a `RefCell`.
#[experimental]
pub fn clone_ref<'b, T>(orig: &Ref<'b, T>) -> Ref<'b, T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this doesn't just call orig.parent.borrow() to avoid code bloat from fail!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to avoid checking if the borrow flag is set to WRITING. LLVM can optimize

let y = x.get();
x.set(y + 1);

into a single inc instruction.

@sfackler
Copy link
Member

Yay for free functions!

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

Successfully merging this pull request may close these issues.

4 participants