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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jul 5 00:07:17 PDT 2018

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
+  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);

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.

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list