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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for the delay.

The patch does not apply cleanly on top of the current head. Mostly conflicts with `switchHeaderSource` for me.
Could you please rebase your change with head and resubmit it?

Another note: current implementation does not seem to handle macros at all (it will only highlight definition of a macro, not its usages).
Do you have an idea on how to support them?



================
Comment at: clangd/ClangdUnit.cpp:784
+/// Finds declarations locations that a given source location refers to.
+class TargetDeclarationFinder : public index::IndexDataConsumer {
+  std::vector<Location> DeclarationLocations;
----------------
This `IndexDataConsumer` effectively does the same thing as `DeclarationLocationsFinder`.
We should reuse the code between those two.

To do that we could:
1. Change `DeclarationLocationsFinder` to return a list of `Decl*`s and `MacroDef*`s that are under caret.
2. In `findDefinition` we should return locations of found `Decl` and `MacroDef`.
3. In `documentHighlights` we would rerun `DocumentHighlightsFinder`to capture locations of `Decls` referenced in step 1.

Do you think this approach would work?


================
Comment at: clangd/ClangdUnit.cpp:1017
+
+  auto DeclLocationsFinder = std::make_shared<TargetDeclarationFinder>(
+      llvm::errs(), SourceLocationBeg, AST.getASTContext(),
----------------
I wonder if we really need to rerun indexer twice here. Probably ok for the first version, but we should definitely think about a faster way to get the `Decl` under cursor than visiting the whole AST. Not sure if it's easy to do with clang's AST, though, maybe we'll need a separate index for that.


================
Comment at: clangd/Protocol.cpp:782
+    llvm::raw_string_ostream(Result) << llvm::format(
+        R"({"range": %s, "kind": %d})", Range::unparse(DH.range).c_str(), DH.kind);
+    return Result;
----------------
It should work without `c_str()`, simply pass result of `unparse` (i.e., `std::string`) to `llvm::format`.
Generally, avoid calling `c_str()` (and `str()` on `StringRef` for that matter).


================
Comment at: clangd/Protocol.h:431
+ */
+struct DocumentHighlight{
+  /**
----------------
There seems to be a missing brace before `{`. I generally run this simple script before submission to make sure my code is always formatted:
```
find "$CLANG_TOOLS_EXTRA_PATH/clangd" -name "*.cpp" -or -name "*.h" -exec clang-format -i --style=LLVM {} \;
find "$CLANG_TOOLS_EXTRA_PATH/unittests/clangd" -name "*.cpp" -or -name "*.h" -exec clang-format -i --style=LLVM {} \ ;
```


https://reviews.llvm.org/D38425