Page MenuHomePhabricator

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

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

Details

Reviewers
sammccall

Diff Detail

Event Timeline

dgoldman created this revision.Mon, Jan 13, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jan 13, 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.Mon, Jan 13, 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.Tue, Jan 14, 12:17 AM
clang-tools-extra/clangd/ClangdLSPServer.cpp
1125

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.Tue, Jan 14, 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.Thu, Jan 23, 8:03 AM
  • Swap to stringset and simpler way of tracking modified files

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

Done