This commit adds the textDocument/didClose handler and adds code to remove ASTUnits
for those documents.
It also changes caching strategy of CompilationDatabase to per-directory rather than per-file.
As part of the change, FixIts were moved to per-file cache.
As a result, they are now synchronized with the mutex used during parsing,
I'm planning to restore the previous behaviour(fixits are immideately availble during parsing)
in a later commit, see TODOs in the code
Details
- Reviewers
krasimir bkramer - Commits
- rG561ba5e66768: [clangd] Remove ASTUnits for closed documents and cache CompilationDatabase per…
rCTE299843: [clangd] Remove ASTUnits for closed documents and cache CompilationDatabase per…
rL299843: [clangd] Remove ASTUnits for closed documents and cache CompilationDatabase per…
Diff Detail
- Build Status
Buildable 5343 Build 5343: arc lint + arc unit
Event Timeline
Looks great! I'm wondering, can you think of ways to test the didClose method similarly to how it's done for other handlers?
clangd/ASTManager.cpp | ||
---|---|---|
203 | Why not make the locking explicit here and don't require handleRequest and parseFileAndPublishDiagnostics to be called under a lock? | |
clangd/ASTManager.h | ||
60 | I'd call the second parameter just Uri. | |
64 | I'd call this Uri. | |
65 | I'd capitalize the member variables. In general, this is the llvm/clang style and the lowercase member variables in Protocol.h are for consistency with the Language Server Protocol. | |
112 | nit: s/immideately/immediately and add a full stop. | |
117 | It's sad that handle request should be called under the lock :( | |
119 | Maybe it's worth noting whether the main or the worker thread calls these during asynchronous execution. | |
131 | nit: missing trailing full stop. | |
132 | I'd rename this lock to ClangObjectLock or something else; its name currently gives the wrong feeling that it's used only for DocDatas. |
Addressed the locking comments. Locking inside the request handlers looks much nicer indeed.
clangd/ASTManager.cpp | ||
---|---|---|
203 | Good point, my initial attempt was aimed at avoiding locking when RunSynchronously is true. But that's not a very useful constraint. |
Regarding the testing. It looks like we have two ways of testing this:
- Add clangd-specific protocol handlers that output useful stats(i.e. currently opened documents), use those in tests.
- Add unit tests that emulate the protocol actions and run the checks in C++.
Option 1 seems preferable, we could probably use the 'workspace/executeCommand' for that.
I'd call the second parameter just Uri.