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.

Sign up for GitHub

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

Jump to bottom

Don't use "weak count" around Weak::from_raw_ptr #74782

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Don't use "weak count" around Weak::from_raw_ptr #74782

merged 1 commit into from
Jul 30, 2020

Conversation

vorner
Copy link
Contributor

@vorner vorner commented Jul 26, 2020

As Rc/Arc::weak_count returns 0 when having no strong counts, this
could be confusing and it's better to avoid using that completely.

Closes #73840.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2020
@vorner
Copy link
Contributor Author

vorner commented Jul 26, 2020

cc @CAD97

dtolnay
dtolnay requested changes Jul 26, 2020
View reviewed changes
src/liballoc/sync.rs Outdated Show resolved Hide resolved
src/liballoc/rc.rs Outdated Show resolved Hide resolved
src/liballoc/sync.rs Outdated Show resolved Hide resolved
src/liballoc/rc.rs Outdated Show resolved Hide resolved
src/liballoc/rc.rs Outdated Show resolved Hide resolved
@CAD97
Copy link
Contributor

CAD97 commented Jul 26, 2020 •
edited
Loading

Rather than try to line-comment, here's my take on the same information; perhaps you can find a happy medium somewhere.

into_raw:

Consumes the Weak<T> and turns it into a raw pointer.

This converts the weak pointer into a raw pointer, suspending ownership of the weak pointer. It can be turned back into the Weak<T>, reclaiming the suspended ownership, via [from_raw].

...

from_raw:

Converts a raw pointer previously created by [into_raw] back into Weak<T>.

...

It reclaims the ownership of the weak reference previously suspended with into_raw (with the exception of pointers created by [new], as these don't actually own any data).

Safety

The pointer must have originated from [into_raw] and there must be a suspended weak reference to reclaim.

It is allowed for the strong count to be 0 at the time of calling this. However, there must have been a previous matching call to [into_raw] to suspend ownership of a weak reference, which is reclaimed by this function (with the exception dangling Weak created by [new], which do not own any data).

...

@vorner
Copy link
Contributor Author

vorner commented Jul 27, 2020

And I hoped navigating the pitfalls of unsafety in the implementation would be the hard part of the feature 😇

OK, I've tried to rewrite it while avoiding any kind of new terminology (like suspending), intuitive descriptions of frozen, etc. I hope it's still readable and no standard-lawyer can prove it does something else than it does.

@bors
Copy link
Contributor

bors commented Jul 28, 2020

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

@vorner
Don't use "weak count" around Weak::from_raw_ptr
ad6d63e 
As `Rc/Arc::weak_count` returns 0 when having no strong counts, this
could be confusing and it's better to avoid using that completely.

Closes rust-lang#73840.
@vorner
Copy link
Contributor Author

vorner commented Jul 28, 2020

Rebased on that big-bang move everything PR. Sorry if it makes the reviewing harder, but I think it should not be a big problem with changes this small and incremental reviewing doesn't really help here.

dtolnay
dtolnay approved these changes Jul 28, 2020
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great.

@dtolnay
Copy link
Member

dtolnay commented Jul 28, 2020

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 28, 2020

📌 Commit ad6d63e has been approved by dtolnay

@bors bors 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 Jul 28, 2020
@bors
Copy link
Contributor

bors commented Jul 28, 2020

⌛ Testing commit ad6d63e with merge 45a9dbd0690ffc6b75d596689c1a2e6841fff5a1...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Jul 28, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 28, 2020
@vorner
Copy link
Contributor Author

vorner commented Jul 28, 2020

I'm trying to look at what failed on CI. I don't know for sure, but it seems like it fails on downloading some docker image?

@jyn514
Copy link
Member

jyn514 commented Jul 30, 2020

This looks like an internal error from crates.io, nothing to do with your PR.

warning: spurious network error (2 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/term/0.6.0/download`, got 502
warning: spurious network error (1 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/term/0.6.0/download`, got 502
error: failed to download from `https://crates.io/api/v1/crates/term/0.6.0/download`

@bors retry

@bors bors 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 Jul 30, 2020
@bors
Copy link
Contributor

bors commented Jul 30, 2020

⌛ Testing commit ad6d63e with merge e532d6541289e12b5586f3a1f63f0153a45e6233...

@bors
Copy link
Contributor

bors commented Jul 30, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 30, 2020
@CAD97
Copy link
Contributor

CAD97 commented Jul 30, 2020 •
edited
Loading

65 completed jobs
in 1h 56m 31s

In what way did this time out?

Oh, x86_64-apple on Azure timed out after five hours: https://github.com/rust-lang-ci/rust/runs/926325983

This can't possibly be the cause of any failures/timeouts, though, it's just a doc change.

@bors rollup=always

(I don't know if r+ rollup sets always but just in case (oh could've sworn bors let me rollup my own PR; guess that's a special case))

@bors
Copy link
Contributor

bors commented Jul 30, 2020

@CAD97: 🔑 Insufficient privileges: not in try users

1 similar comment
@bors
Copy link
Contributor

bors commented Jul 30, 2020

@CAD97: 🔑 Insufficient privileges: not in try users

@Mark-Simulacrum
Copy link
Member

@bors retry

@bors bors 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 Jul 30, 2020
@Manishearth Manishearth mentioned this pull request Jul 30, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2020
@bors
Auto merge of rust-lang#74957 - Manishearth:rollup-3wudwlg, r=Manishe…
cfc572c 
…arth

Rollup of 9 pull requests

Successful merges:

 - rust-lang#74751 (Clean up E0730 explanation)
 - rust-lang#74782 (Don't use "weak count" around Weak::from_raw_ptr)
 - rust-lang#74835 (Clean up E0734 explanation)
 - rust-lang#74871 (Enable docs on dist-x86_64-musl)
 - rust-lang#74905 (Avoid bool-like naming)
 - rust-lang#74907 (Clean up E0740 explanation)
 - rust-lang#74915 (rustc: Ignore fs::canonicalize errors in metadata)
 - rust-lang#74934 (Improve diagnostics when constant pattern is too generic)
 - rust-lang#74951 (Cherry-pick the release notes for 1.45.1)

Failed merges:

r? @ghost
@bors bors merged commit 4637968 into rust-lang:master Jul 30, 2020
@vorner vorner deleted the weak-into-raw-cnt-doc branch July 31, 2020 06:10
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.

Weak::from_raw is practically unusable
9 participants
@vorner @rust-highfive @CAD97 @bors @dtolnay @rust-log-analyzer @jyn514 @Mark-Simulacrum @cuviper