This is an archive of the discontinued LLVM Phabricator instance.

Remove ASTUnits for closed documents and cache CompilationDatabase per directory in clangd.
ClosedPublic

Authored by ilya-biryukov on Apr 6 2017, 2:04 AM.

Details

Summary

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

Event Timeline

ilya-biryukov created this revision.Apr 6 2017, 2:04 AM
ilya-biryukov added a project: Restricted Project.Apr 6 2017, 2:10 AM
ilya-biryukov added a subscriber: cfe-commits.
krasimir edited edge metadata.Apr 6 2017, 5:07 AM

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
202

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.

118

nit: s/immideately/immediately and add a full stop.

121

It's sad that handle request should be called under the lock :(

123

Maybe it's worth noting whether the main or the worker thread calls these during asynchronous execution.

134

nit: missing trailing full stop.

135

I'd rename this lock to ClangObjectLock or something else; its name currently gives the wrong feeling that it's used only for DocDatas.

Minor fixes.
Fixed variable name issues and comment spelling errors.

ilya-biryukov marked 6 inline comments as done.Apr 7 2017, 1:26 AM

Moved locking of ClangObjectLock into request handlers.

ilya-biryukov marked 3 inline comments as done.Apr 7 2017, 1:58 AM

Addressed the locking comments. Locking inside the request handlers looks much nicer indeed.

clangd/ASTManager.cpp
202

Good point, my initial attempt was aimed at avoiding locking when RunSynchronously is true. But that's not a very useful constraint.

ilya-biryukov marked an inline comment as done.Apr 7 2017, 2:08 AM

Regarding the testing. It looks like we have two ways of testing this:

  1. Add clangd-specific protocol handlers that output useful stats(i.e. currently opened documents), use those in tests.
  2. 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.

Fixed conflicts with the upstream

krasimir accepted this revision.Apr 10 2017, 6:42 AM

Looks good!

This revision is now accepted and ready to land.Apr 10 2017, 6:42 AM
This revision was automatically updated to reflect the committed changes.