Skip to content

Conversation

@Noratrieb
Copy link
Contributor

The nomicon has been outdated about this since 2015,
see rust-lang/nomicon#363 for the update.
We don't need to tell dropck that we are owning a T as it
can already infer that from the generic parameter in the drop impl.

cc @Cassy343 who added it

@Cassy343
Copy link
Contributor

Cassy343 commented Aug 4, 2022

Could this be modified to just remove the comment and explain instead that we'll need something like this if a #[may_dangle]-like thing ever gets stabilized?

Copy link
Owner

@faern faern left a comment

Choose a reason for hiding this comment

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

Looks good overall. Please also remove the _dropck field from Sender and Receiver then. They shoud be equally not needed.

I don't think we need to drag these along just in case #[may_dangle] becomes a thing. Because if that becomes a thing I'm assuming it would be opt in, and any such opting in would need to make sure the ownership is correctly marked at that point.

@faern
Copy link
Owner

faern commented Aug 14, 2022

Looks like you force pushed a new commit that is identical?

The nomicon has been outdated about this since 2015,
see rust-lang/nomicon#363 for the update.
We don't need to tell dropck that we are owning a T as it
can already infer that from the generic parameter in the drop impl.
@Noratrieb
Copy link
Contributor Author

oh

@faern faern merged commit 3cee01f into faern:master Aug 14, 2022
@faern
Copy link
Owner

faern commented Aug 14, 2022

Thank you for the contribution!

@Noratrieb Noratrieb deleted the patch-1 branch August 14, 2022 10:06
@faern
Copy link
Owner

faern commented Aug 14, 2022

Aah. It should have been removed from the changelog as well! https://github.com/faern/oneshot/blob/3cee01f3ff91afa7691357494f87cbd3d2727446/CHANGELOG.md#fixed

I'll do that directly.

@faern
Copy link
Owner

faern commented Aug 14, 2022

b84c624

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.

3 participants