This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Build dynamic index and use it for code completion.
ClosedPublic

Authored by ioeric on Dec 15 2017, 6:14 AM.

Diff Detail

Event Timeline

ioeric created this revision.Dec 15 2017, 6:14 AM
ioeric updated this revision to Diff 127493.Dec 19 2017, 4:22 AM
  • Merge with updated D41281
  • Fix broken merge
ioeric updated this revision to Diff 127544.Dec 19 2017, 8:59 AM
  • Merge with D41289.
  • Merge with origin/master
sammccall accepted this revision.Dec 19 2017, 9:17 AM

OK, this is pretty clean now! :-)

clangd/ClangdServer.cpp
141

A comment explaining when this runs and what it does might be nice.

Also this doesn't seem like an ideal long-term solution: rebuilding an index can be slow (less the symbol extraction, and more the rebuilding of index data structures) and we may be able to index on less critical paths.
Probably also worth a comment.

clangd/ClangdUnit.cpp
364

CppFile doesn't need to pass the path, do you want [FileName, ASTCallback](const Context &C, ParsedAST *AST) { ASTCallback(C, FileName, AST); }

clangd/ClangdUnitStore.h
28

synchronously... maybe mention this will block diagnostics and doing expensive things would be bad

clangd/tool/ClangdMain.cpp
98

llvm:🆑:Hidden, if it's experimental?

unittests/clangd/CodeCompleteTests.cpp
561

for this test, I don't see a clear sign that the results come from the index.
Is there a way we know this that you can point out?

For the second test we can tell because there's no #include, but it could use a comment

584

This repeated setup is a bit ugly but I'm planning to pull out a helper for this anyway.

This revision is now accepted and ready to land.Dec 19 2017, 9:17 AM
ioeric updated this revision to Diff 127554.Dec 19 2017, 9:57 AM
ioeric marked 4 inline comments as done.
  • Address review comments.

Thanks for the quick review!

clangd/ClangdUnit.cpp
364

The downside is 1) we need to worry about the life time of FileName 2) we need another type for the new callback, so I am inclined to simply forward the callback.

This revision was automatically updated to reflect the committed changes.