Skip to content

RefCell: document panics in Clone, PartialEq, PartialOrd, Ord. - #48365

Merged
bors merged 2 commits into
rust-lang:masterfrom
Centril:docs/document-refcell-panics
Feb 28, 2018
Merged

RefCell: document panics in Clone, PartialEq, PartialOrd, Ord.#48365
bors merged 2 commits into
rust-lang:masterfrom
Centril:docs/document-refcell-panics

Conversation

@Centril

@CentrilCentril commented Feb 20, 2018

Copy link
Copy Markdown
Contributor

This fixes#47400 by adding:

/// # Panics////// Panics if the value is currently mutably borrowed.

to said impls. They may panic since they call .borrow().

@CentrilCentril added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2018
@rust-highfive

rust-highfive commented Feb 20, 2018

Copy link
Copy Markdown
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2018
@Centril

Centril commented Feb 22, 2018

Copy link
Copy Markdown
ContributorAuthor

r? @steveklabnik

Re-assigning to a T-doc member =)

@emilyalbini

emilyalbini commented Feb 26, 2018

Copy link
Copy Markdown
Member

@steveklabnik ping from the release team!

frewsxcv
frewsxcv reviewed Feb 27, 2018
View reviewed changes
Comment threadsrc/libcore/cell.rs Outdated
impl<T: ?Sized + PartialEq> PartialEq for RefCell<T> {
/// # Panics
///
/// Panics if the value is currently mutably borrowed.

@frewsxcvfrewsxcvFeb 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the functions that take an other like this one, what do you think about using this wording:

Panics if the value in either RefCell is currently borrowed.

This wording is taken from RefCell::swap

@CentrilCentrilFeb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sounds like a great improvement 👍

steveklabnik
steveklabnik approved these changes Feb 27, 2018
View reviewed changes

@steveklabniksteveklabnik left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

r=me after changing to @frewsxcv 's wording; consistency is important and i think it's slightly clearer. Thanks so much for this PR!

@Centril

Centril commented Feb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

@steveklabnik: Will do in a jiffy =)

@Centril

Centril commented Feb 27, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

And fixed.

r=@steveklabnik

@frewsxcv

frewsxcv commented Feb 27, 2018

Copy link
Copy Markdown
Contributor

@bors r+ rollup

thanks!

@bors

bors commented Feb 27, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit f8ebb3f has been approved by frewsxcv

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Feb 28, 2018
@kennytm
Rollup merge ofrust-lang#48365- Centril:docs/document-refcell-panic…
c599463
…s, r=frewsxcv
RefCell: document panics in Clone, PartialEq, PartialOrd, Ord.
This fixesrust-lang#47400 by adding:
```rust
/// # Panics
///
/// Panics if the value is currently mutably borrowed.
```
to said impls. They may panic since they call `.borrow()`.
@kennytmkennytm mentioned this pull request Feb 28, 2018
Merged
bors added a commit that referenced this pull request Feb 28, 2018
@bors
Auto merge of#48608- kennytm:rollup, r=kennytm
0ff9872
Rollup of 15 pull requests
- Successful merges: #48266, #48321, #48365, #48381, #48450, #48473, #48479, #48484, #48488, #48497, #48541, #48548, #48558, #48560, #48565
- Failed merges:
@bors
bors merged commit f8ebb3f into rust-lang:masterFeb 28, 2018
@Centril
Centril deleted the docs/document-refcell-panics branch February 28, 2018 15:59
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@frewsxcvfrewsxcvfrewsxcv left review comments
@steveklabniksteveklabniksteveklabnik approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@steveklabniksteveklabnik

Labels

A-docsArea: Documentation for any part of the project, including the compiler, standard library, and toolsC-enhancementCategory: An issue proposing an enhancement or a PR with one.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

libstd/rustdoc: Document potential panics in RefCell where .borrow() is used implicitly in impls

7 participants

@Centril@rust-highfive@emilyalbini@frewsxcv@bors@steveklabnik@alexcrichton