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

[Merged by Bors] - Fix Option<NonSend<T>> and Option<NonSendMut<T>> #2757

Closed
wants to merge 11 commits into from

Conversation

bilsen
Copy link
Contributor

@bilsen bilsen commented Aug 31, 2021

Objective

Fix Option<NonSend<T>> to work when T isn't Send
Fix Option<NonSendMut<T>> to work when T isnt in the world.

Solution

Simple two row fix, properly initialize T in OptionNonSendState and remove T: Component bound for Option<NonSendMut<T>>
also added a rudimentary test

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 31, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior and removed S-Needs-Triage This issue needs to be labelled labels Aug 31, 2021
@alice-i-cecile
Copy link
Member

The comment fixes look like this got pulled in from a different PR perhaps? They look good to merge, but it would be cleaner with just the two fix commits and the tests in there.

bilsen added 2 commits August 31, 2021 18:04
This reverts commit 0f89477.
This reverts commit 10eb1f2.
@bilsen
Copy link
Contributor Author

bilsen commented Aug 31, 2021

The comment fixes look like this got pulled in from a different PR perhaps? They look good to merge, but it would be cleaner with just the two fix commits and the tests in there.

yea thats true, fixed that.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 31, 2021
@cart
Copy link
Member

cart commented Aug 31, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 31, 2021
# Objective
Fix `Option<NonSend<T>>` to work when T isn't `Send`
Fix `Option<NonSendMut<T>>` to work when T isnt in the world.

## Solution
Simple two row fix, properly initialize T in `OptionNonSendState` and remove `T: Component` bound for `Option<NonSendMut<T>>`
also added a rudimentary test


Co-authored-by: Ïvar Källström <[email protected]>
@bors bors bot changed the title Fix Option<NonSend<T>> and Option<NonSendMut<T>> [Merged by Bors] - Fix Option<NonSend<T>> and Option<NonSendMut<T>> Aug 31, 2021
@bors bors bot closed this Aug 31, 2021
bilsen added a commit to bilsen/bevy that referenced this pull request Sep 2, 2021
# Objective
Fix `Option<NonSend<T>>` to work when T isn't `Send`
Fix `Option<NonSendMut<T>>` to work when T isnt in the world.

## Solution
Simple two row fix, properly initialize T in `OptionNonSendState` and remove `T: Component` bound for `Option<NonSendMut<T>>`
also added a rudimentary test


Co-authored-by: Ïvar Källström <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants