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

Check the need for active.set(false); in Entity::clean #1024

Closed
AlmasB opened this issue Apr 22, 2021 · 0 comments
Closed

Check the need for active.set(false); in Entity::clean #1024

AlmasB opened this issue Apr 22, 2021 · 0 comments
Assignees
Milestone

Comments

@AlmasB
Copy link
Owner

AlmasB commented Apr 22, 2021

It appears active is always false at that point, in which case the call is redundant and can be removed:

// TODO: we probably don't need to set active false again, since we do that in markForRemoval?
active.set(false);
}

I have looked at call sites for clean() in GameWorld and they are always preceded by marForRemoval()

void markForRemoval() {
onNotActive.run();
active.set(false);
}

So, active is false. This code is in a robust-critical section, so I'd like a second review.

@adambocco could you double check the clean() call sites and see if it is somehow possible for active to be true before it reaches clean.

@AlmasB AlmasB self-assigned this Apr 22, 2021
@AlmasB AlmasB added this to the FXGL 17 milestone Sep 21, 2021
@AlmasB AlmasB closed this as completed in 8ea6b75 Nov 13, 2021
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

No branches or pull requests

1 participant