This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix a reference invalidation
ClosedPublic

Authored by kadircet on Jan 14 2019, 1:52 AM.

Event Timeline

kadircet created this revision.Jan 14 2019, 1:52 AM
ilya-biryukov added inline comments.Jan 14 2019, 2:03 AM
clangd/index/Background.cpp
521

We should avoid modifying the container we're iterating over instead.
I suggest we separate the queue and the return value into two separate vectors.

We would pop() from the queue and push() to the result vector, therefore making bugs like this impossible.

kadircet updated this revision to Diff 181512.Jan 14 2019, 2:13 AM
  • Change traversal logic to use two different containers
kadircet marked an inline comment as done.Jan 14 2019, 2:13 AM
ilya-biryukov added inline comments.Jan 14 2019, 2:27 AM
clangd/index/Background.cpp
488

This reference makes it just as easy to access the vector we'll be modifying inside the loop.
Can we avoid modifying the values inside the vector completely? I.e. ideally we'll have only push_back into the vector and no modifications of the internal references?

ilya-biryukov added inline comments.Jan 14 2019, 2:32 AM
clangd/index/Background.cpp
488

Ah, but we don't modify the vector anymore.
Could you leave a comment that Dependencies is not modified further in the loop body, so it's fine to have a reference inside it?

ilya-biryukov accepted this revision.Jan 14 2019, 2:33 AM

LGTM, but please add a comment the vector is not modified (it might be useful as a reminder in case we want to change the code later)

This revision is now accepted and ready to land.Jan 14 2019, 2:33 AM
kadircet updated this revision to Diff 181524.Jan 14 2019, 3:25 AM
  • Add a comment
This revision was automatically updated to reflect the committed changes.