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