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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
