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

Arc downcast #50836

Merged
merged 4 commits into from
Jun 1, 2018
Merged

Arc downcast #50836

merged 4 commits into from
Jun 1, 2018

Conversation

jsgf
Copy link
Contributor

@jsgf jsgf commented May 17, 2018

Implement downcast for Arc<Any + Send + Sync> as part of #44608, and gated by the same rc_downcast feature.

This PR is mostly lightly-edited cut'n'paste.

This has two additional changes:

  • The downcast implementation needs Any + Send + Sync implementations for is and Debug, and I added downcast_ref and downcast_mut for completeness/consistency. (Can these be insta-stabilized?)
  • At @SimonSapin's suggestion, I converted Arc and Rc to use NonNull::cast to avoid an unsafe block in each which tidied things up nicely.

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2018
@pietroalbini
Copy link
Member

r? @SimonSapin

@SimonSapin
Copy link
Contributor

Stability attributes on trait impls don’t really work, so those impls are effectively insta-stable if both the trait and type are stable.

Any + Send + Sync methods can be made unstable and on principle should, but since they’re identical to existing Any and Any + Send methods I’m ok with landing them as insta-stable.

In both cases though, the attribute should still be accurate and say since = "1.28.0". That probably requires having a different feature name, maybe any_send_sync_methods or something.

@SimonSapin SimonSapin added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2018
@jsgf
Copy link
Contributor Author

jsgf commented May 17, 2018

OK, I made them

    #[stable(feature = "any_send_sync_methods", since = "1.28.0")]

@SimonSapin
Copy link
Contributor

SimonSapin commented May 18, 2018

r=me on the diff. Let’s also get team sign off on insta-stable APIs:

  • impl Debug for Any + Send + Sync
  • <Any+Send+Sync>::is:<T: Any>(&self) -> bool
  • <Any+Send+Sync>::downcast_ref:<T: Any>(&self) -> Option<&T>
  • <Any+Send+Sync>::downcast_mut:<T: Any>(&mut self) -> Option<&mut T>

@rfcbot fcp merge

@SimonSapin SimonSapin added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2018
@SimonSapin
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented May 18, 2018

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 18, 2018
@jsgf jsgf force-pushed the arc-downcast branch 2 times, most recently from 082e2ba to f7d06f8 Compare May 18, 2018 15:26
@@ -971,6 +972,44 @@ unsafe impl<#[may_dangle] T: ?Sized> Drop for Arc<T> {
}
}

impl Arc<Any + Send + Sync> {
#[inline]
Copy link
Contributor

Choose a reason for hiding this comment

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

Attributes tend to go below doc comments

Copy link
Contributor Author

@jsgf jsgf May 18, 2018

Choose a reason for hiding this comment

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

This is a copy of the corresponding code in Rc. I agree it looks strange.

@rfcbot
Copy link

rfcbot commented May 26, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 26, 2018
@SimonSapin
Copy link
Contributor

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented May 31, 2018

📌 Commit f7d06f8 has been approved by SimonSapin

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 31, 2018
@bors
Copy link
Contributor

bors commented May 31, 2018

🔒 Merge conflict

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 31, 2018
jsgf added 4 commits May 31, 2018 13:27
Implement `is`, `downcast_ref`, `downcast_mut` and `Debug` for
`Any + Send + Sync`.
We only need to implement it for `Any + Send + Sync` because in practice
that's the only useful combination for `Arc` and `Any`.

Implementation for rust-lang#44608 under the `rc_downcast` feature.
This avoids an `unsafe` block in each case.
Make the Any+Send+Sync examples use the right trait bounds, and fix a small whitespace issue.
@jsgf
Copy link
Contributor Author

jsgf commented May 31, 2018

I rebased onto current master (though I didn't need to resolve any conflicts).

@SimonSapin
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 31, 2018

📌 Commit 87c3d7b has been approved by SimonSapin

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 31, 2018
@bors
Copy link
Contributor

bors commented Jun 1, 2018

⌛ Testing commit 87c3d7b with merge b7a9d4e...

bors added a commit that referenced this pull request Jun 1, 2018
Arc downcast

Implement `downcast` for `Arc<Any + Send + Sync>` as part of #44608, and gated by the same `rc_downcast` feature.

This PR is mostly lightly-edited cut'n'paste.

This has two additional changes:
- The `downcast` implementation needs `Any + Send + Sync` implementations for `is` and `Debug`, and I added `downcast_ref` and `downcast_mut` for completeness/consistency. (Can these be insta-stabilized?)
- At @SimonSapin's suggestion, I converted `Arc` and `Rc` to use `NonNull::cast` to avoid an `unsafe` block in each which tidied things up nicely.
@bors
Copy link
Contributor

bors commented Jun 1, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: SimonSapin
Pushing b7a9d4e to master...

@bors bors merged commit 87c3d7b into rust-lang:master Jun 1, 2018
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 5, 2018
@pthariensflame
Copy link
Contributor

Should this be tagged relnotes?

@jsgf jsgf deleted the arc-downcast branch June 7, 2018 01:45
@pietroalbini pietroalbini added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 7, 2018
@pietroalbini
Copy link
Member

Yep, thanks @pthariensflame!

@SimonSapin
Copy link
Contributor

For the Any + Send + Sync parts, yes. The Arc parts are still unstable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants