malaperle added inline comments.
================ Comment at: clangd/FindSymbols.cpp:181 +/// Finds document symbols in the main file of the AST. +class DocumentSymbolsConsumer : public index::IndexDataConsumer { + ASTContext &AST; ---------------- sammccall wrote: > I guess the alternative here is to use SymbolCollector and then convert > Symbol->SymbolInformation (which we should already have for workspace/symbol). > > I also guess there's some divergence in behavior, which is why you went this > route :-) > Mostly in filtering? (I'd think that for e.g. printing, we'd want to be > consistent) > > Do you think it's worth adding enough customization to SymbolCollector to > make it usable here? Even if it was making `shouldFilterDecl` a > std::function, there'd be some value in unifying the rest. WDYT? I put a bit of the justification in the summary, perhaps you missed it or maybe did you think it was not enough of a justification? > An AST-based approach is used to retrieve the document symbols rather than an > in-memory index query. The index is not an ideal fit to achieve this because > of > the file-centric query being done here whereas the index is suited for > project-wide queries. Document symbols also includes more symbols and need to > keep the order as seen in the file. It's not a clear cut decision but I felt that there was more diverging parts than common and that it would be detrimental to SymbolCollector in terms of added complexity. Differences: - We need a different way to filter (as you suggested) - Don't need anything about completion - Don't need to distinguish declaration vs definition or canonical declaration - Don't need a map<USR, Symbol>, we just need them in same order as they appear and SymbolCollector should be free to store them in whichever order for purposes of proving a project-wide collection of symbols - Don't want to track references - DocumentSymbols needs symbols in main files Common things: - Both need to generate Position/Uri from a SourceLocation. But they can both call sourceLocToPosition. - Both need to generate a symbol name from Decl&. But they can both call AST.h's printQualifiedName (I fixed this in a new version). - Both use/extend a IndexDataConsumer. Not worth sharing the same code path just to not have to create a class definition IMO. Let me know if that makes sense to you, otherwise I can try to make another version that uses SymbolCollector. ================ Comment at: unittests/clangd/FindSymbolsTests.cpp:447 + EXPECT_THAT(getSymbols(FilePath), + ElementsAre(AllOf(QName("Tmpl"), WithKind(SymbolKind::Struct)), + AllOf(QName("Tmpl::x"), WithKind(SymbolKind::Field)), ---------------- sammccall wrote: > in a perfect world, I think the template definitions might be printed > `Tmpl<T>`, `Tmpl<int>`. Not sure about `Tmpl::x` vs `Tmpl<T>::x` though. WDYT? > > (Not necessarily worth doing this patch, keep it simple) I'm thinking `Tmpl<class T>`, `Tmpl<int>`, so that we can more easily distinguish between a primary and specialization, especially in partial specializations and when the generic type might not be just a single letter. And `Tmpl<class T>::x`. Maybe this is too much :) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47846 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits