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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
