This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Also cache failures while indexing
ClosedPublic

Authored by kadircet on Jul 1 2019, 4:17 AM.

Details

Summary

Clangd currently doesn't cache any indexing failures, which results in
retrying those failed files even if their contents haven't changed.

Event Timeline

kadircet created this revision.Jul 1 2019, 4:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 4:18 AM

Change looks sensible, just wondering if we should keep the main-file symbols/refs, and whether we can simplify/unify a bit.

clang-tools-extra/clangd/index/Background.cpp
359

couldn't we have a much more unified handling of error/non-error case, by simply dropping all non-main-file shards at this point? (in case of error)

In theory we're processing them a bit, but it practice it seems like they're empty.

clang-tools-extra/clangd/index/IndexAction.cpp
161

I'm pretty sure the include graph is basically good in this case (up to the possibly-missing files). So we should at least send that, so invalidation works (or can work in the future).

Also, we agreed not to overwrite the symbols for the included headers' shards, but what about the main file itself? Surely better to have something than nothing. (The "something" is roughly clang's recovery, which is what AST-based completions rely on anyway)

More generally, do we need filtering logic at both ends?
It seems like this file could just pass through the data it has, along with the info on whether there was an error (somehow)

163

comment belongs in the if, not the else

kadircet updated this revision to Diff 207296.Jul 1 2019, 7:11 AM
kadircet marked 4 inline comments as done.
  • Address comments
kadircet added inline comments.Jul 1 2019, 7:13 AM
clang-tools-extra/clangd/index/IndexAction.cpp
161

Left the error detection behavior same(checking for uncompilable errors)

sammccall accepted this revision.Jul 1 2019, 7:32 AM

Thanks!

clang-tools-extra/clangd/index/Background.cpp
140

(I'd suggest inlining this again as it's fairly clear straight-line code, but up to you)

317

Maybe add a FIXME that we should store other contents too, but only if the current shard contents are missing or also had errors.

511

why do we want to skip (maybe) rebuilding the index in this case?

511

I think this is now a "warning" rather than an error, maybe log something like "failed to compile {Cmd.Filename}, index may be incomplete"

This revision is now accepted and ready to land.Jul 1 2019, 7:32 AM
kadircet updated this revision to Diff 207316.Jul 1 2019, 8:07 AM
kadircet marked 5 inline comments as done.
  • Address comments
kadircet added inline comments.Jul 1 2019, 9:09 AM
clang-tools-extra/clangd/index/Background.cpp
317

I agree with storing contents if they are missing, but does it make sense to replace one broken shard information with other?

511

actually, we don't want to skip. I had written that part with the old assumption; that is, "we don't get any symbol info when compilation fails".

Still LG

clang-tools-extra/clangd/index/Background.cpp
317

I'd say it makes sense if the content has changed.

i.e:

bool replace() {
  if old == null
    return true
  if !new.isTU && new.errors && !old.errors
    return false
  return new.digest != old.digest
}

(don't let this block you landing it, up to you)

kadircet updated this revision to Diff 207484.Jul 2 2019, 12:38 AM
kadircet marked 2 inline comments as done.
  • Update fixme to mention error case in addition to the missing files.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2019, 2:52 AM