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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
