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 an incorrect iteration with a workList #6177

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jkwak-work
Copy link
Collaborator

We cannot modify workList while iterating it, because its type List is actually an array container.

We cannot modify workList while iterating it, because its type `List` is
actually an array container.
@jkwak-work jkwak-work added the pr: non-breaking PRs without breaking changes label Jan 24, 2025
@jkwak-work jkwak-work requested a review from kaizhangNV January 24, 2025 23:49
@jkwak-work jkwak-work self-assigned this Jan 24, 2025
@jkwak-work jkwak-work requested a review from a team as a code owner January 24, 2025 23:49
@csyonghe
Copy link
Collaborator

csyonghe commented Jan 24, 2025

Actually it is fine to append items to a List while iterating it, and the newly appended item will be processed.

This is one major divergence from std containers, which doesn't allow this behavior. In practice we find allowing this makes a lot of code simpler (and potentially faster).

Is the code leading to any crashes or bugs? The for loop should be iterating with an integer index instead of using the iterator, if the iterator is just a pointer (and that pointer will change if the list reallocates).

@kaizhangNV kaizhangNV closed this Jan 24, 2025
@kaizhangNV
Copy link
Contributor

Is the code leading to any crashes or bugs? The for loop should be iterating with an integer index instead of using the iterator, if the iterator is just a pointer (and that pointer will change if the list reallocates).

Yea, I have a same ask here

@kaizhangNV kaizhangNV reopened this Jan 24, 2025
@jkwak-work
Copy link
Collaborator Author

It was causing a crash.
I will try with index as suggested.

@jkwak-work jkwak-work enabled auto-merge (squash) January 25, 2025 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants