[PATCH] D38425: [clangd] Document highlights for clangd

[Ilya Biryukov via Phabricator via cfe-commits]

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