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

Make Entities::needs_flush take &self instead of &mut self #6490

Closed

Conversation

2ne1ugly
Copy link
Contributor

@2ne1ugly 2ne1ugly commented Nov 5, 2022

Objective

Solution

  • Use load() instead of get_mut() at the atomic i64
  • Also change verify_flushed as &self

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Nov 5, 2022
@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 Nov 5, 2022
@DJMcNab
Copy link
Member

DJMcNab commented Nov 5, 2022

This only taking &self makes these methods not quite useless, but much more error prone. That's because using the result of this without being in a context where you have exclusive access to the entities means that the window where the results are guaranteed to be valid is empty, since another thread could do another operation inbetween the check and use of this property to make it invalid.

verify_flushed provides an easy way to see this, since it purports to confirm that the Entities is flushed but doesn't actually ensure this for any length of time.

In this case, we're using &mut T not as 'can be mutated', but merely as validation of exclusivity; clippy doesn't understand the difference between those (as the language doesn't make the distinction). Clippy is wrong here. This is the first case of rust-lang/rust-clippy#5112

Additionally, of course, this adds more unneeded atomic loads into a place, although that's a secondary concern.

@alice-i-cecile alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Nov 5, 2022
@2ne1ugly
Copy link
Contributor Author

2ne1ugly commented Nov 5, 2022

I guess the another way to avoid the clippy warning is to just do it the regular way:
call function outside of debug and pass as variable

then we could "silence" the warning

@2ne1ugly 2ne1ugly deleted the remove-mut-on-entities-needs-flush branch November 5, 2022 17:00
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-Code-Quality A section of code that is hard to understand or change D-Complex Quite challenging from either a design or technical perspective. Ask for help!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clippy::debug-assert-with-mut-call in Entities::verify_flushed()
4 participants