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 RefCell::{try_borrow, try_borrow_mut} #35425

Merged
merged 1 commit into from
Aug 9, 2016
Merged

Implement RefCell::{try_borrow, try_borrow_mut} #35425

merged 1 commit into from
Aug 9, 2016

Conversation

apasel422
Copy link
Contributor

@apasel422
Copy link
Contributor Author

The error structs may want to implement some additional traits, but those can be added later.

#[derive(Clone, Debug)]
#[unstable(feature = "try_borrow", issue = "35070")]
pub struct BorrowError {
_inner: (),
Copy link
Contributor

Choose a reason for hiding this comment

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

What _inner field for?
You can write just

pub struct BorrowError {
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This way it can't be constructed outside the module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I copied this from the RFC itself, and it can always be removed before being stabilized.

@alexcrichton
Copy link
Member

alexcrichton commented Aug 8, 2016

Thanks @apasel422! To be maximally conservative as well, could you add a lifetime parameter to the two error types so it's connected to &self of the RefCell itself? Right now they can just be defined as:

struct Error<'a, T> {
    inner: marker::PhantomData<&'a RefCell<T>>,
}

Also, could you be sure to add Error implementations for these types in libstd as well?

@apasel422
Copy link
Contributor Author

@alexcrichton Sure. Do you still want a Clone impl for these, in that case?

@alexcrichton
Copy link
Member

Nah it should be fine to omit for now I think

@apasel422
Copy link
Contributor Author

Updated.

not be exposed publicly",
issue = "0")]
#[doc(hidden)]
pub fn __description(&self) -> &str {
Copy link
Member

Choose a reason for hiding this comment

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

Despite this being how std::num does it in a few locations, that's just because the variants in core aren't available in std. For something like these errors it should be fine to just put this error string directly into std, no need to worry about reducing the duplication of this string literal!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@alexcrichton
Copy link
Member

@bors: r+ a20a1db

Thanks!

@bors
Copy link
Collaborator

bors commented Aug 9, 2016

⌛ Testing commit a20a1db with merge c2b03f8...

bors added a commit that referenced this pull request Aug 9, 2016
Implement `RefCell::{try_borrow, try_borrow_mut}`

CC #35070

r? @alexcrichton
@bors bors merged commit a20a1db into rust-lang:master Aug 9, 2016
@apasel422 apasel422 deleted the refcell branch August 9, 2016 12:27
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.

5 participants