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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks great! Would like to get your thoughts on reusing/not reusing SymbolCollector.



================
Comment at: clangd/FindSymbols.cpp:181
+/// Finds document symbols in the main file of the AST.
+class DocumentSymbolsConsumer : public index::IndexDataConsumer {
+  ASTContext *
----------------
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?


================
Comment at: clangd/SourceCode.cpp:194
+  if (!llvm::sys::path::is_absolute(FilePath)) {
+    if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
+      log("Could not turn relative path to absolute: " + FilePath);
----------------
the common case when tryGetRealPathName() is empty seems to be when it's a file that was part of the preamble we're reusing.
Does this fallback tend to give the same answer in that case? (If so, great! I know some other places we should reuse this function!)


================
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)),
----------------
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)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47846