Implementation of Document Highlights Request as described in LSP.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
clangd/ClangdUnit.cpp | ||
---|---|---|
1029 ↗ | (On Diff #124391) | Last |
1069 ↗ | (On Diff #124391) | 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 ↗ | (On Diff #124391) | 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 ↗ | (On Diff #124391) | 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 ↗ | (On Diff #123848) | Still there |
clangd/ClangdServer.cpp | ||
513 ↗ | (On Diff #124834) | Other functions don't crash in this case anymore, we should follow the same pattern here (see findDefinitions for an example) |
524 ↗ | (On Diff #124834) | We should return an error this case. |
clangd/ClangdUnit.cpp | ||
1062 ↗ | (On Diff #124834) | This function should be private. |
1096 ↗ | (On Diff #124834) | We should not return default-initialized Locations from this function. |
1165 ↗ | (On Diff #124834) | 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 ↗ | (On Diff #124834) | Please also compare kind here, to make this operator consistent with operator==. |
test/clangd/initialize-params-invalid.test | ||
23 ↗ | (On Diff #124834) | NIT: Misindent |
test/clangd/initialize-params.test | ||
23 ↗ | (On Diff #124834) | 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 ↗ | (On Diff #124951) | "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 | ||
1088 ↗ | (On Diff #124951) | const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart)); if (!F) return llvm::None; |
1098 ↗ | (On Diff #124951) | maybe move the check earlier? see comment above. |
clangd/Protocol.h | ||
601 ↗ | (On Diff #124951) | Use /// like other structs |
clangd/ClangdServer.cpp | ||
---|---|---|
526 ↗ | (On Diff #124951) | missing return? llvm::make_error<llvm::StringError>( |
clangd/ClangdServer.cpp | ||
---|---|---|
528 ↗ | (On Diff #125228) | Invalid |
530 ↗ | (On Diff #125228) | 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 | ||
372 ↗ | (On Diff #125228) | Too many new lines. Only keep one. |
clangd/Protocol.h | ||
568 ↗ | (On Diff #125228) | Use /// like other places |
575 ↗ | (On Diff #125228) | 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 ↗ | (On Diff #125346) | 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 ↗ | (On Diff #125346) | 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 | ||
---|---|---|
365 ↗ | (On Diff #125346) | 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 | ||
---|---|---|
249 ↗ | (On Diff #125619) | Mentioning AST in this comment is weird, macros have nothing to do with the AST. We should remove/rephrase the comment. |
clangd/Protocol.h | ||
562 ↗ | (On Diff #125619) | NIT: remove this empty comment line and all the others. |
581 ↗ | (On Diff #125619) | This comparison does not provide a total order. |