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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Sorry, I thought I'd accepted this already!



================
Comment at: clangd/ClangdServer.cpp:467
+  };
+  WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB)));
+}
----------------
hover -> documentSymbols


================
Comment at: clangd/FindSymbols.cpp:188
+public:
+  DocumentSymbolsConsumer(raw_ostream &OS, ASTContext &AST) : AST(AST) {}
+  std::vector<SymbolInformation> takeSymbols() { return std::move(Symbols); }
----------------
OS is unused


================
Comment at: clangd/FindSymbols.cpp:203
+
+  bool shouldFilterDecl(const NamedDecl *ND) {
+    if (!ND || ND->isImplicit())
----------------
nit: rename to shouldIncludeSymbol or something? "filter" is slightly ambiguous, and a negation. (We've made this change in SymbolCollector recently)


================
Comment at: clangd/FindSymbols.cpp:229
+    // We should be only be looking at "local" decls in the main file.
+    if (SourceMgr.getMainFileID() != SourceMgr.getFileID(NameLoc)) {
+      // Even thought we are visiting only local (non-preamble) decls,
----------------
nit: isWrittenInMainFile(NameLoc)


================
Comment at: clangd/FindSymbols.cpp:234
+    }
+    // assert(SourceMgr.getMainFileID() == SourceMgr.getFileID(NameLoc));
+    const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(ASTNode.OrigD);
----------------
remove


================
Comment at: clangd/FindSymbols.cpp:256
+
+    Symbols.push_back({Name, SK, L, Scope});
+    return true;
----------------
nit: for readability, could we declare a local SymbolInformation above, fill in the fields by name, and then move it into Symbols?

a) it's easier to see that the right values go into the right slots, b), it's easier to search (find references!) for how fields are populated.


================
Comment at: clangd/FindSymbols.cpp:268
+  index::IndexingOptions IndexOpts;
+  // We don't need references, only declarations so that is makes a nice
+  // outline of the document.
----------------
applying the filter is fine, but note that SystemSymbolFilter only applies to included symbols in -isystem headers (I only just noticed that myself). So the comment is a little misleading, I would drop it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47846