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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

> Another note: current implementation does not seem to handle macros at all (it will only highlight definition of a macro, not its usages).

Current implementation still doesn't highlight macro references, only definitions. I'd either disable `documentHighlight` for macros for the initial version or come up with a way to implement `documentHighlight` for them.
I don't think `Preprocessor` currently stores information about all macro references, so we'll need to either store a separate index for that or rerun preprocessor to get this information.



================
Comment at: clangd/ClangdUnit.cpp:1017
+
+  auto DeclLocationsFinder = std::make_shared<TargetDeclarationFinder>(
+      llvm::errs(), SourceLocationBeg, AST.getASTContext(),
----------------
Nebiroth wrote:
> ilya-biryukov wrote:
> > 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.
> Yeah I realise it's definitely not the fastest way to go about it. Maybe there is a way to stop handling occurrences once the highlighted declaration is found. This would be the most straightforward way of improving the performance of the feature without re-writing a new AST indexer.
That only helps if we get lucky, average case and worst case would still be slow.
Anyway, this commit can certainly go in without this change.


================
Comment at: clangd/ClangdUnit.cpp:879
 public:
+  std::vector<const Decl *> Decls;
+  std::vector<MacroInfo *> MacroInfos;
----------------
Let's not store  `DeclarationLocations` anymore. If we have `Decl`s and `MacroInfo`s, we can move the code getting their `Location`s out of the `DeclarationLocationsFinder`.


================
Comment at: clangd/ClangdUnit.cpp:880
+  std::vector<const Decl *> Decls;
+  std::vector<MacroInfo *> MacroInfos;
   DeclarationLocationsFinder(raw_ostream &OS,
----------------
Please make the fields private, e.g. put them before `SearchedLocations`.


================
Comment at: clangd/ClangdUnit.cpp:976
+public:
+  std::vector<const Decl *> Decls;
+  std::vector<MacroInfo *> MacroInfos;
----------------
Make these fields private.
Don't copy `std::vector`s, store references to them instead.


================
Comment at: clangd/ClangdUnit.cpp:1003
+    const SourceManager &SourceMgr = AST.getSourceManager();
+    if (std::find(Decls.begin(), Decls.end(), D) != Decls.end()) {
+      SourceLocation Begin, End;
----------------
Please rewrite 
```
if (cond) {
  // long code
}
return true;
```
to 
```
if (!cond)
  return true;

// long code
```

This reduces nesting and makes code easier to read.
See [[ https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code | style guide ]].


================
Comment at: clangd/Protocol.h:431
+ */
+struct DocumentHighlight{
+  /**
----------------
ilya-biryukov wrote:
> 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 {} \ ;
> ```
BTW, the script had an error. (missing some parens)
```
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 {} \ ;

```


================
Comment at: test/clangd/initialize-params.test:24
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
+
+
----------------
Redundant newlines at the end of the file.


https://reviews.llvm.org/D38425