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
