This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Only re-open files if their flags changed
ClosedPublic

Authored by dgoldman on Jan 13 2020, 2:01 PM.

Diff Detail

Unit TestsFailed

Event Timeline

dgoldman created this revision.Jan 13 2020, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2020, 2:01 PM

Unit tests: fail. 61795 tests passed, 1 failed and 781 were skipped.

failed: Clangd.Clangd/did-change-configuration-params.test

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

dgoldman updated this revision to Diff 237791.Jan 13 2020, 3:08 PM
  • Fix broken did-change-configuration test

Unit tests: pass. 61796 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

sammccall added inline comments.Jan 14 2020, 12:17 AM
clang-tools-extra/clangd/ClangdLSPServer.cpp
1125–1129

nit: llvm::StringSet<> or DenseSet<string> are the usual type here

1126

this is a clever technique. (Why not just use compilationDatabaseChanges directly? I suppose because then you have to deal more with path canonicalization?)

it risks having the CDB change concurrently and reloading those files too, though.
I guess there's not much harm in it. But in that case, why aren't we just permanently subscribing to CDB changes and re-parsing affected files? Lack of a thread to do it on?

dgoldman added inline comments.Jan 14 2020, 7:50 AM
clang-tools-extra/clangd/ClangdLSPServer.cpp
1126

Yeah I think compilationDatabaseChanges would be equivalent to what is here now, I can just swap to that.

For the perma subscribe I wasn't sure of the threading. If addDocument is thread safe I think we're okay to just call Server->AddDocument from whatever thread without holding a mutex?

Let's make the minimal change here and land this.

clang-tools-extra/clangd/ClangdLSPServer.cpp
1126

ClangdServer isn't threadsafe, that's a good point.

Yeah I think compilationDatabaseChanges would be equivalent to what is here now, I can just swap to that.

This is the minimal change, let's do that.

dgoldman updated this revision to Diff 239907.Jan 23 2020, 8:03 AM
  • Swap to stringset and simpler way of tracking modified files

Let's make the minimal change here and land this.

Done

sammccall accepted this revision.Jan 25 2020, 7:12 AM
This revision is now accepted and ready to land.Jan 25 2020, 7:12 AM
This revision was automatically updated to reflect the committed changes.