[PATCH] D52222: [clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC.

[Sam McCall via Phabricator via cfe-commits]

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

Glorious! Think the APIs could be simplified/clarified a little further, but up to you.



================
Comment at: clangd/index/FileIndex.cpp:24
+indexSymbols(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
+             llvm::ArrayRef<Decl *> DeclsToIndex, bool IsIndexMainAST,
+             llvm::ArrayRef<std::string> URISchemes) {
----------------
assert DeclsToIndex is empty if !IsIndexMainAST?


================
Comment at: clangd/index/FileIndex.h:77
+  /// Update symbols from main file \p Path with symbols in \p TopLevelDecls.
+  void updateMain(PathRef Path, ASTContext &AST,
+                  std::shared_ptr<Preprocessor> PP,
----------------
can't this just take PathRef + ParsedAST?
(same data, but clearer semantics)


================
Comment at: clangd/index/FileIndex.h:114
+std::pair<SymbolSlab, RefSlab>
+indexMainDecls(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
+               llvm::ArrayRef<Decl *> Decls,
----------------
again, take ParsedAST here?


================
Comment at: clangd/index/FileIndex.h:118
+
+/// Idex all declarations in \p AST and all macro definitions in \p PP.
+/// If URISchemes is empty, the default schemes in SymbolCollector will be used.
----------------
This isn't quite right I think - we don't index the symbols that only occur in the main file.


================
Comment at: clangd/index/FileIndex.h:120
+/// If URISchemes is empty, the default schemes in SymbolCollector will be used.
 std::pair<SymbolSlab, RefSlab>
 indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
----------------
this shouldn't return any refs, just return SymbolSlab here


================
Comment at: clangd/index/FileIndex.h:121
 std::pair<SymbolSlab, RefSlab>
 indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
          llvm::ArrayRef<std::string> URISchemes = {});
----------------
indexPreamble would be clearer I think.


================
Comment at: unittests/clangd/TestTU.cpp:51
 
 std::unique_ptr<SymbolIndex> TestTU::index() const {
   auto AST = build();
----------------
this should return FileIndex instead I think, with both preamble and main index.
But this requires fixing callers (since FileIndex isn't an index).
Leave a fixme?

(Maybe I should just bite the bullet and expose the MergedIndex class so we don't have to deal with these indirections...)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52222