[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol

[Marc-Andre Laperle via Phabricator via cfe-commits]

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 *
----------------
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