malaperle requested changes to this revision. malaperle added inline comments. This revision now requires changes to proceed.
================ Comment at: clangd/ClangdLSPServer.cpp:67 }}}}); + if (Params.rootUri && !Params.rootUri->file.empty()) ---------------- extra line ================ Comment at: clangd/ClangdLSPServer.cpp:242 + + auto Items = Server + .findDocumentHighlights(Params.textDocument.uri.file, ---------------- Items -> Highlights? ================ Comment at: clangd/ClangdServer.h:291 + /// Get document highlights for a symbol hovered on. + Tagged<std::vector<DocumentHighlight>> findDocumentHighlights(PathRef File, + Position Pos); ---------------- can this thing fail? Should it be Expected<Tagged... ?? ================ Comment at: clangd/ClangdUnit.cpp:942 /// Finds declarations locations that a given source location refers to. class DeclarationLocationsFinder : public index::IndexDataConsumer { std::vector<Location> DeclarationLocations; ---------------- I think this class should be exactly the same as the code hover patch, so changes from there and the comments on that patch should apply here too. ================ Comment at: clangd/ClangdUnit.cpp:944 std::vector<Location> DeclarationLocations; + std::vector<const Decl *> DeclarationDecls; + std::vector<const MacroInfo *> DeclarationMacroInfos; ---------------- just Decls? ================ Comment at: clangd/ClangdUnit.cpp:945 + std::vector<const Decl *> DeclarationDecls; + std::vector<const MacroInfo *> DeclarationMacroInfos; + std::vector<DocumentHighlightKind> Kinds; ---------------- just MacroInfos? ================ Comment at: clangd/ClangdUnit.cpp:946 + std::vector<const MacroInfo *> DeclarationMacroInfos; + std::vector<DocumentHighlightKind> Kinds; const SourceLocation &SearchedLocation; ---------------- remove, see comment below. ================ Comment at: clangd/ClangdUnit.cpp:961 + std::sort(DeclarationDecls.begin(), DeclarationDecls.end()); + auto last = + std::unique(DeclarationDecls.begin(), DeclarationDecls.end()); ---------------- Last ================ Comment at: clangd/ClangdUnit.cpp:969 + // Don't keep the same location multiple times. + // This can happen when nodes in the AST are visited twice. + std::sort(DeclarationMacroInfos.begin(), DeclarationMacroInfos.end()); ---------------- This comment needs to be updated (like the code hover patch) ================ Comment at: clangd/ClangdUnit.cpp:971 + std::sort(DeclarationMacroInfos.begin(), DeclarationMacroInfos.end()); + auto last = + std::unique(DeclarationMacroInfos.begin(), DeclarationMacroInfos.end()); ---------------- Last ================ Comment at: clangd/ClangdUnit.cpp:992 index::IndexDataConsumer::ASTNodeInfo ASTNode) override { + if (isSearchedLocation(FID, Offset)) { ---------------- extra line ================ Comment at: clangd/ClangdUnit.cpp:996 + DeclarationDecls.push_back(D); + DocumentHighlightKind Kind; + switch (Roles) { ---------------- I think you need to do this in DocumentHighlightsFinder not here. Each occurrence can have its kind, not just the user selected location. ================ Comment at: clangd/ClangdUnit.cpp:1013 + std::vector<const Decl *> getDeclarationDecls() { return DeclarationDecls; }; + std::vector<const MacroInfo *> getDeclarationMacroInfos() { return DeclarationMacroInfos; }; ---------------- I don't think you need any of those getters. Only use the "takes" ================ Comment at: clangd/ClangdUnit.cpp:1079 +/// Finds document highlights that a given FileID and file offset refers to. +class DocumentHighlightsFinder : public index::IndexDataConsumer { ---------------- It's not "a given FileID and file", the finder is given a list of Decls ================ Comment at: clangd/ClangdUnit.cpp:1122 +Location clangd::addDeclarationLocation(ParsedAST &AST, SourceRange SR) { + const SourceManager &SourceMgr = AST.getASTContext().getSourceManager(); ---------------- This is duplicated code with getDeclarationLocation. It also doesn't use tryGetRealPathName I also don't think this is called?? ================ Comment at: clangd/ClangdUnit.cpp:1165 +Location clangd::getDeclarationLocation(ParsedAST &AST, + const SourceRange &ValSourceRange) { ---------------- I don't think this is called? ================ Comment at: clangd/ClangdUnit.cpp:1191 + +DocumentHighlight clangd::addHighlightLocation(ParsedAST &AST, SourceRange SR, + DocumentHighlightKind Kind) { ---------------- This doesn't add or have a location in its signature so I think it should be named differently. DocumentHighligh getDocumentHighlight(Decl*) ? Or addDocumentHighlight(Decl*) if you move it in DocumentHighlightsFinder. ================ Comment at: clangd/ClangdUnit.cpp:1232 + + std::vector<const Decl *> TempDecls = DeclLocationsFinder->getDeclarationDecls(); + std::vector<DocumentHighlightKind> TempKinds = ---------------- call takeDecls. TempDecls -> SelectedDecls? ================ Comment at: clangd/ClangdUnit.cpp:1233 + std::vector<const Decl *> TempDecls = DeclLocationsFinder->getDeclarationDecls(); + std::vector<DocumentHighlightKind> TempKinds = + DeclLocationsFinder->getKinds(); ---------------- remove? see comment below ================ Comment at: clangd/ClangdUnit.cpp:1243 + + std::vector<DocumentHighlight> HighlightLocations; + ---------------- DocumentHighlights. They are not Locations (so they are not confused with the struct in Protocol) ================ Comment at: clangd/ClangdUnit.cpp:1245 + + for (unsigned I = 0; I < DocHighlightsFinder->getSourceRanges().size(); I++) { + HighlightLocations.push_back( ---------------- replace all this code (1242-1265) by moving it in DocumentHighlightsFinder? then call takeHighlights. ================ Comment at: clangd/ClangdUnit.cpp:1251 + + for (unsigned I = 0; I < DeclLocationsFinder->getDeclarationMacroInfos().size(); I++) { + HighlightLocations.push_back(addHighlightLocation( ---------------- I think we should remove macro handling for now, it doesn't highlight occurrences anyway. ================ Comment at: clangd/ClangdUnit.cpp:1265 + HighlightLocations.erase(last, HighlightLocations.end()); + return std::move(HighlightLocations); +} ---------------- remove std::move, returning such local object will use copy elision ================ Comment at: clangd/ClangdUnit.h:318 +Location addDeclarationLocation(ParsedAST &AST, SourceRange SR); + ---------------- remove? ================ Comment at: clangd/Protocol.h:637 + DocumentHighlightKind kind = DocumentHighlightKind::Text; + int test = 3; + ---------------- remove ================ Comment at: clangd/Protocol.h:639 + + friend bool operator<(const DocumentHighlight &LHS, + const DocumentHighlight &RHS) { ---------------- needed? ================ Comment at: clangd/ProtocolHandlers.cpp:49 }; - } // namespace ---------------- revert change ================ Comment at: clangd/ProtocolHandlers.h:34 virtual ~ProtocolCallbacks() = default; - virtual void onInitialize(Ctx C, InitializeParams &Params) = 0; ---------------- revert change https://reviews.llvm.org/D38425 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits