Page MenuHomePhabricator

[clangd] Fix updated file detection logic in indexing
ClosedPublic

Authored by kadircet on Fri, Jan 11, 4:43 AM.

Details

Summary

Files without any symbols were never marked as updated during indexing, which resulted in failure while writing shards for these files.

This patch fixes the logic to mark files that are seen for the first time but don't contain any symbols as updated.

Diff Detail

Event Timeline

kadircet created this revision.Fri, Jan 11, 4:43 AM
ilya-biryukov added inline comments.Mon, Jan 14, 1:50 AM
clangd/index/Background.cpp
318

If this the only reason we're traversing Index.Sources? If so, I suggest removing this comment and traversing Files instead.
This would make the code more straightforward and would definitely cost us only a negligible performance penalty.

kadircet updated this revision to Diff 181540.Mon, Jan 14, 5:13 AM
kadircet marked an inline comment as done.
  • Change the logic to detect updated files to take new files without any symbols into account.
kadircet retitled this revision from [clangd] Do not override contents of the shards without modification to [clangd] Fix updated file detection logic in indexing.Mon, Jan 14, 5:16 AM
kadircet edited the summary of this revision. (Show Details)
kadircet added inline comments.Mon, Jan 14, 5:21 AM
clangd/index/Background.cpp
318

Well actually, in addition to that I was traversing source files rather than updated files to make sure we write down shards even for sources without any symbols or refs in them. Because current logic in FileFilter didn't take files without any symbols into account.

So this is actually rather about detecting if a file is already up-to-date or not, changing that logic to take new files without any symbols into account.

kadircet updated this revision to Diff 181544.Mon, Jan 14, 6:14 AM
  • Change FileFilter and update logic to do not care about if there are any symbols coming from a file.
kadircet updated this revision to Diff 181545.Mon, Jan 14, 6:16 AM
  • Update comments

This looks neat, thanks for the change!
Just a few NITs from my side

clangd/index/Background.cpp
264

NIT: maybe rephrase to "different or missing from DigestsSnapshot"?

280

Use value type, since it's an iterator:
auto DigestIt = DigestsSnapshot.find or const auto DigestIt = …

287–288

Maybe avoid extra lookups?

auto FileIt = Files.find(DeclPath);
if (FileIt != Files.end())
  FileIt->Symbols.insert(&Sym);
298–299

Maybe avoid extra lookups here too?

307–308

Same here, maybe avoid extra lookups?

ilya-biryukov accepted this revision.Mon, Jan 14, 9:34 AM

LGTM after the NITs are fixed. Unless there are other significant changes planned that I need to look at.

This revision is now accepted and ready to land.Mon, Jan 14, 9:34 AM
kadircet updated this revision to Diff 181735.Tue, Jan 15, 1:04 AM
kadircet marked 6 inline comments as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.