[PATCH] D41289: [clangd] Build dynamic index and use it for code completion.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

OK, this is pretty clean now! :-)



================
Comment at: clangd/ClangdServer.cpp:139
+      FileIdx(BuildDynamicSymbolIndex ? new FileIndex() : nullptr),
+      Units(FileIdx
+                ? [this](const Context &Ctx, PathRef Path,
----------------
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.


================
Comment at: clangd/ClangdUnit.cpp:617
+      new CppFile(FileName, std::move(Command), StorePreamblesInMemory,
+                  std::move(PCHs), std::move(ASTCallback)));
 }
----------------
CppFile doesn't need to pass the path, do you want `[FileName, ASTCallback](const Context &C, ParsedAST *AST) { ASTCallback(C, FileName, AST); }`


================
Comment at: clangd/ClangdUnitStore.h:28
 public:
+  /// \p ASTCallback is called when a file is parsed.
+  explicit CppFileCollection(ASTParsedCallback ASTCallback)
----------------
synchronously... maybe mention this will block diagnostics and doing expensive things would be bad


================
Comment at: clangd/tool/ClangdMain.cpp:98
+        "use index built from symbols in opened files"),
+    llvm::cl::init(false));
+
----------------
llvm::cl::Hidden,  if it's experimental?


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:561
 
+TEST(CompletionTest, ASTIndexOneFile) {
+  MockFSProvider FS;
----------------
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


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:584
+TEST(CompletionTest, ASTIndexMultiFile) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
----------------
This repeated setup is a bit ugly but I'm planning to pull out a helper for this anyway.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41289