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

Fix possible crash #4673

Merged
merged 2 commits into from
May 7, 2024
Merged

Fix possible crash #4673

merged 2 commits into from
May 7, 2024

Conversation

kygov
Copy link
Contributor

@kygov kygov commented May 7, 2024

Pull Request Prelude

Changes Proposed

Fixed broken iteration.

Issues addressed:
Crash because removing element from map and iterating.

Copy link
Contributor

@EvilHero90 EvilHero90 left a comment

Choose a reason for hiding this comment

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

This doesn't work either as expected, I'd suggest to do it the safe way with 2 loops

std::vector<std::string> invalid;
for (auto& event : creatureEvents) {
	if (event.second.getScriptId() == 0) {
		invalid.push_back(event.first);
	}
}
for (auto& toRemove : invalid) {
	creatureEvents.erase(toRemove);
}
invalid.clear();

@kygov
Copy link
Contributor Author

kygov commented May 7, 2024

This doesn't work either as expected, I'd suggest to do it the safe way with 2 loops

std::vector<std::string> invalid;
for (auto& event : creatureEvents) {
	if (event.second.getScriptId() == 0) {
		invalid.push_back(event.first);
	}
}
for (auto& toRemove : invalid) {
	creatureEvents.erase(toRemove);
}
invalid.clear();

What's not working as expected?

@EvilHero90
Copy link
Contributor

EvilHero90 commented May 7, 2024

What's not working as expected?

tried it on different online debuggers, the first one I tried gave weird outcomes, others work just fine
sorry my mistake

@EvilHero90 EvilHero90 merged commit 098ed9c into otland:master May 7, 2024
19 checks passed
@nekiro
Copy link
Member

nekiro commented May 7, 2024

could be std::erase_if

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants