ilya-biryukov added inline comments.
================
Comment at: clangd/index/FileIndex.cpp:48
+ if (TopLevelDecls) { // index main AST, set occurrence flag.
+ CollectorOpts.OccurrenceFilter = SymbolOccurrenceKind::Declaration |
----------------
hokein wrote:
> 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?
Either a dedicated methods or a separate parameter LG.
And it's probably ok to do it with a follow-up change if you want to keep the
scope of this patch smaller. That should be a pretty small change overall, so
it should be fine either way.
================
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,
----------------
hokein wrote:
> 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.
Not really, that was just a suggestion. Feel free to ignore if the current
version looks better to you.
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
