Implementation of Document Highlights Request as described in LSP.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 12607 Build 12607: arc lint + arc unit
Event Timeline
clangd/ClangdUnit.cpp | ||
---|---|---|
1029 | Last | |
1069 | Don't call getLocForEndOfToken here again. This is the reason why sometimes it includes an extra character. Just to explain a bit, supposed we have the first call to getLocForEnd in handleDeclOccurence will compute the position at |
Minor code cleanup
Fixed highlights sometimes obtaining one too many characters inside range
Updated test
clangd/ClangdLSPServer.cpp | ||
---|---|---|
246 | I get a test failure here because there is an assertion that the Expected<> needs to be checked. I can't really think of any failure case right now where we wouldn't just return an empty array of highlights. But I think it's better for consistency and future-proofing to keep the Expected<>. if (!Highlights) { C.replyError(ErrorCode::InternalError, llvm::toString(Highlights.takeError())); return; } |
clangd/ClangdLSPServer.cpp | ||
---|---|---|
246 | Just to be clear, the error I see in the output is: It asserts when in ClangdLSPServer::onDocumentHighlight, referencing Highlights->Value |
Added verification for llvm::Expected in onDocumentHighlight
Removed unused variable in ClangdUnit
clangd/ClangdLSPServer.cpp | ||
---|---|---|
67 | Still there | |
clangd/ClangdServer.cpp | ||
513 | Other functions don't crash in this case anymore, we should follow the same pattern here (see findDefinitions for an example) | |
524 | We should return an error this case. | |
clangd/ClangdUnit.cpp | ||
1062 | This function should be private. | |
1096 | We should not return default-initialized Locations from this function. | |
1165 | Let's add a comment that we don't currently handle macros. It's ok for the first iteration, having documentHighlights for Decls is useful enough to get it in. | |
clangd/Protocol.h | ||
621 | Please also compare kind here, to make this operator consistent with operator==. | |
test/clangd/initialize-params-invalid.test | ||
23 | NIT: Misindent | |
test/clangd/initialize-params.test | ||
23 | NIT: Misindent |
Minor code cleanup getDeclarationLocation now returns llvm::Optional operator< for DocumentHighlight struct now properly compares the kind field
very minor comments
clangd/ClangdServer.h | ||
---|---|---|
288 | "for a symbol hovered on." It doesn't have to be a symbol and the user doesn't have to hover on it. So maybe just "for a given position" | |
clangd/ClangdUnit.cpp | ||
1087 | const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart)); if (!F) return llvm::None; | |
1097 | maybe move the check earlier? see comment above. | |
clangd/Protocol.h | ||
601 | Use /// like other structs |
clangd/ClangdServer.cpp | ||
---|---|---|
526 | missing return? llvm::make_error<llvm::StringError>( |
clangd/ClangdServer.cpp | ||
---|---|---|
528 | Invalid | |
530 | It trips here with assertions enabled because we haven't checked the previous "Value" for error before, sigh. How about this: std::vector<DocumentHighlight> Result; llvm::Optional<llvm::Error> Err; Resources->getAST().get()->runUnderLock([Pos, &Result, &Err, this](ParsedAST *AST) { if (!AST) Err = llvm::make_error<llvm::StringError>("Invalid AST", llvm::errc::invalid_argument); Result = clangd::findDocumentHighlights(*AST, Pos, Logger); }); if (Err) return std::move(*Err); return make_tagged(Result, TaggedFS.Tag); The tests pass for me with this. | |
clangd/Protocol.cpp | ||
566 | Too many new lines. Only keep one. | |
clangd/Protocol.h | ||
607 | Use /// like other places | |
614 | Use /// like other places |
Minor code cleanup
Refactored findDocumentHighlights() to make tests pass when assertions are enabled
Can you also reformat the code too with clang-format? I think there's a few things in ClangdUnit.h/.cpp
clangd/ClangdServer.cpp | ||
---|---|---|
521 | We should do like findDefinitions here: if (!Resources) return llvm::make_error<llvm::StringError>( "findDocumentHighlights called on non-added file", llvm::errc::invalid_argument); | |
528 | Woops, I forgot the return here if it's an error, so it should be: if (!AST) { Err = llvm::make_error<llvm::StringError>("Invalid AST", llvm::errc::invalid_argument); return; } |
Added error returns in findDocumentHighlights
Ran clang-format on ClangdUnit.h, .cpp and ClangdServer.cpp
Sorry I forgot to submit this additional comment in my last review pass :(
clangd/Protocol.cpp | ||
---|---|---|
568 | static_cast<int>(DH.kind) |
This looks good to me. @ilya-biryukov, @sammccall Any objections?
I think we can do further iterations later to handle macros and other specific cases (multiple Decls at the position, etc) . It's already very useful functionality.
Overall looks good, just a few minor comments.
Let me know if need someone to land this patch for you after you fix those.
clangd/ClangdUnit.cpp | ||
---|---|---|
961 | Mentioning AST in this comment is weird, macros have nothing to do with the AST. We should remove/rephrase the comment. | |
clangd/Protocol.h | ||
601 | NIT: remove this empty comment line and all the others. | |
620 | This comparison does not provide a total order. |