Skip to content

Add missing Send/Sync impls#225

Merged
zesterer merged 1 commit intoamethyst:masterfrom
Imberflur:trustcell-fix
Feb 13, 2023
Merged

Add missing Send/Sync impls#225
zesterer merged 1 commit intoamethyst:masterfrom
Imberflur:trustcell-fix

Conversation

@Imberflur
Copy link
Contributor

@Imberflur Imberflur commented Feb 11, 2023

Forgot these in #223!

zesterer
zesterer reviewed Feb 12, 2023
View reviewed changes
src/cell.rs Outdated
}

// SAFETY: `Ref<'_, T> acts as a reference.
unsafe impl<'a, T: ?Sized + 'a> Sync for Ref<'a, T> where T: Sync {}
Copy link
Contributor

@zesterer zesterer Feb 12, 2023

Choose a reason for hiding this comment

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

I think this is correct, but can we express the bounds as where for<'a> &'a T: Sync and where for<'a> &'a T: Send instead so we're matching the impls with those of actual references?

Copy link
Contributor Author

@Imberflur Imberflur Feb 12, 2023

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor Author

@Imberflur Imberflur Feb 12, 2023

Choose a reason for hiding this comment

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

Amended commit

Copy link
Contributor Author

@Imberflur Imberflur Feb 12, 2023

Choose a reason for hiding this comment

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

Actually I would like to double check if this works when T is not 'static

Copy link
Contributor Author

@Imberflur Imberflur Feb 12, 2023

Choose a reason for hiding this comment

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

seems fine

@zesterer
Copy link
Contributor

zesterer commented Feb 13, 2023

Miri failure? Hopefully nothing important

@Imberflur
Copy link
Contributor Author

Imberflur commented Feb 13, 2023

Miri failure? Hopefully nothing important

This is to be expected for now. MetaTable does some very questionable stuff with vtable pointers. I haven't ran into tests (in specs) that fail Miri due to this yet so I'm not focusing on it (but I would like to have reminder of Miri failing). Unfortunately, github CI lacks ability to have a CI check that is allowed to fail without showing red X https://github.com/orgs/community/discussions/15452. IIRC I think MetaTable is used for potential scripting support.

@zesterer zesterer merged commit 9708b7f into amethyst:master Feb 13, 2023
@Imberflur Imberflur deleted the trustcell-fix branch February 13, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zesterer zesterer zesterer left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Imberflur @zesterer