ilya-biryukov requested changes to this revision.
ilya-biryukov added inline comments.
This revision now requires changes to proceed.
================
Comment at: clangd/ClangdLSPServer.cpp:67
}}}});
+
if (Params.rootUri && !Params.rootUri->file.empty())
----------------
malaperle wrote:
> extra line
Still there
================
Comment at: clangd/ClangdServer.cpp:513
+ auto FileContents = DraftMgr.getDraft(File);
+ assert(FileContents.Draft &&
+ "findDocumentHighlights is called for non-added document");
----------------
Other functions don't crash in this case anymore, we should follow the same
pattern here (see `findDefinitions` for an example)
================
Comment at: clangd/ClangdServer.cpp:524
+ if (!AST)
+ return;
+ Result = clangd::findDocumentHighlights(*AST, Pos, Logger);
----------------
We should return an error this case.
================
Comment at: clangd/ClangdUnit.cpp:1062
+
+ DocumentHighlight getDocumentHighlight(SourceRange SR,
+ DocumentHighlightKind Kind) {
----------------
This function should be private.
================
Comment at: clangd/ClangdUnit.cpp:1096
+ Range R = {Begin, End};
+ Location L;
+ if (const FileEntry *F =
----------------
We should not return default-initialized `Locations` from this function.
Return `llvm::Optional<Location>` and handle the case when `getFileEntryForID`
returns null by returning `llvm::None`.
================
Comment at: clangd/ClangdUnit.cpp:1165
indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
+ DeclMacrosFinder, IndexOpts);
----------------
Let's add a comment that we don't currently handle macros. It's ok for the
first iteration, having `documentHighlights` for `Decl`s is useful enough to
get it in.
================
Comment at: clangd/Protocol.h:621
+ const DocumentHighlight &RHS) {
+ return std::tie(LHS.range) < std::tie(RHS.range);
+ }
----------------
Please also compare `kind` here, to make this operator consistent with
`operator==`.
`std::tie(LHS.range, LHS.kind) < std::tie(RHS.range, RHS.kind)` should work.
If it doesn't, converting enums to int should help: `std::tie(LHS.range,
int(LHS.kind)) < std::tie(RHS.range, int(RHS.kind))`
================
Comment at: test/clangd/initialize-params-invalid.test:23
# CHECK-NEXT: "documentFormattingProvider": true,
+# CHECK-NEXT: "documentHighlightProvider": true,
# CHECK-NEXT: "documentOnTypeFormattingProvider": {
----------------
NIT: Misindent
================
Comment at: test/clangd/initialize-params.test:23
# CHECK-NEXT: "documentFormattingProvider": true,
+# CHECK-NEXT: "documentHighlightProvider": true,
# CHECK-NEXT: "documentOnTypeFormattingProvider": {
----------------
NIT: Misindent
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
