hokein added a comment. Thanks for the comments.
================ Comment at: clangd/index/FileIndex.cpp:48 + if (TopLevelDecls) { // index main AST, set occurrence flag. + CollectorOpts.OccurrenceFilter = SymbolOccurrenceKind::Declaration | ---------------- ilya-biryukov wrote: > 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? This is a hacky way to detect whether we are indexing main AST or preamble AST -- because the current `FileIndex::update(PathRef Path, ...)` interface doesn't contain this information, and we use it both for `PreambleAST` and `MainAST` indexing in the client side (`ParsingCallback`). I think we might want two dedicated methods (`updateMainAST`, and `updatePreambleAST`) in `FileSymbol`, and I think we should address it in a follow-up patch. WDYT? ================ Comment at: clangd/index/FileIndex.h:50 + std::vector<std::shared_ptr<SymbolOccurrenceSlab>> allOccurrenceSlabs() const; + ---------------- sammccall wrote: > sammccall wrote: > > This return type seems inconsistent with the design of this class. > > The purpose of the `shared_ptr<vector<symbol_slab>>` in `allSymbols` is to > > avoid exposing the concrete storage. > > The analogue would I guess be `shared_ptr<DenseMap<SymbolID, > > vector<SymbolOccurrence*>>>`. > > > > The cost of creating that does seem a little gross though. > allOccurrences > (name should reflect the semantics, not the type) > The cost of creating that does seem a little gross though. Previously I tried to avoid the cost of iterating/copying all occurrences. After some investigations, it seems that the number of symbol occurrences is small (~500 for `SemaDecl.cpp`, < 100 for `CodeComplete.cpp`). The cost should not be too expensive. ================ 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, ---------------- ilya-biryukov wrote: > 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. I think the type of `pair` explicitly express its meaning well enough? And this function is exposed for unittests, I'm happy to do the change if you insist. ================ Comment at: clangd/index/MemIndex.cpp:36 auto Idx = llvm::make_unique<MemIndex>(); - Idx->build(getSymbolsFromSlab(std::move(Slab))); + Idx->build(getSymbolsFromSlab(std::move(Slab)), {}); return std::move(Idx); ---------------- sammccall wrote: > we should find a way to avoid having functions implicitly zero out parts of > the index like this! Made it explicit. ================ Comment at: clangd/index/Merge.cpp:95 + Static->findOccurrences(Req, [&](const SymbolOccurrence& O) { + if (llvm::is_contained(SeenFiles, O.Location.FileURI)) + return; ---------------- ilya-biryukov wrote: > `llvm::is_contained` seems to be linear time, maybe replace with hash-table > lookup. > Or am I missing something? Good catch, I thought it had a trait for `Map`, `Set`, but it turns out that `is_contained` is only a `std::find` wrapper :( 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