ilya-biryukov added a comment.
The memory usage looks good. Some NITs and a major consideration wrt to the
implementation of merging occurences from dynamic and static index.
================
Comment at: clangd/index/FileIndex.cpp:48
+ if (TopLevelDecls) { // index main AST, set occurrence flag.
+ CollectorOpts.OccurrenceFilter = SymbolOccurrenceKind::Declaration |
----------------
There is an implicit assumption (`TopLevelDecls != null` iff indexing the main
file AST in dynamic index) that does not seem quite obvious.
Maybe add two overloads (for indexing the main AST and for indexing the
preamble AST) or add an extra parameter to be explicit more about those choices?
================
Comment at: clangd/index/FileIndex.cpp:74
+ if (!Slab) {
FileToSlabs.erase(Path);
+ } else {
----------------
NIT: maybe remove braces around single-statement branches?
================
Comment at: clangd/index/FileIndex.cpp:126
auto Slab = llvm::make_unique<SymbolSlab>();
- *Slab = indexAST(*AST, PP, TopLevelDecls, URISchemes);
- FSymbols.update(Path, std::move(Slab));
+ auto IndexResults = indexAST(*AST, PP, TopLevelDecls, URISchemes);
+ *Slab = std::move(IndexResults.first);
----------------
Why not `std::tie(*Slab, *OccurenceSlab) = indexAST(...)`?
I assume this should work and give the optimal performance
================
Comment at: clangd/index/FileIndex.h:45
+ void update(PathRef Path, std::unique_ptr<SymbolSlab> Slab,
+ std::unique_ptr<SymbolOccurrenceSlab> Occurrences = nullptr);
----------------
Maybe avoid default arguments? Having clients pass nullptr explicitly seems
like the right thing to do
================
Comment at: clangd/index/FileIndex.h:100
/// level decls obtained from \p AST are indexed.
-SymbolSlab
+std::pair<SymbolSlab, SymbolOccurrenceSlab>
indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
----------------
Maybe use a struct to allow named fields? Would mean a tiny amount of extra
code in `indexAST`, but should make the client code slightly nicer.
================
Comment at: clangd/index/MemIndex.h:27
+ build(std::shared_ptr<std::vector<const Symbol *>> Symbols,
+ std::vector<std::shared_ptr<SymbolOccurrenceSlab>> OccurrenceSlabs =
{});
----------------
Maybe remove the default parameter? Making the callers specify the occurrences
explicitly could make their code more straight-forward to follow.
================
Comment at: clangd/index/Merge.cpp:88
+ // in seen files.
+ // FIXME: If a file has been updated, and there are no occurrences indexed
+ // in dynamic index, stale results are still returned (from static index).
----------------
Do we want to fix it before checking this in? As discussed offline, this seems
to be a limitation that could affect the design (FileOccurrences vs
SymbolOccurrence in the symbol interface). In case we want to avoid refactoring
the interfaces later.
================
Comment at: clangd/index/Merge.cpp:95
+ Static->findOccurrences(Req, [&](const SymbolOccurrence& O) {
+ if (llvm::is_contained(SeenFiles, O.Location.FileURI))
+ return;
----------------
`llvm::is_contained` seems to be linear time, maybe replace with hash-table
lookup.
Or am I missing something?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
