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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.


================
Comment at: clangd/ClangdLSPServer.cpp:67
         }}}});
+
   if (Params.rootUri && !Params.rootUri->file.empty())
----------------
extra line


================
Comment at: clangd/ClangdLSPServer.cpp:242
+
+  auto Items = Server
+                   .findDocumentHighlights(Params.textDocument.uri.file,
----------------
Items -> Highlights?


================
Comment at: clangd/ClangdServer.h:291
+  /// Get document highlights for a symbol hovered on.
+  Tagged<std::vector<DocumentHighlight>> findDocumentHighlights(PathRef File,
+                                                                Position Pos);
----------------
can this thing fail? Should it be Expected<Tagged... ??


================
Comment at: clangd/ClangdUnit.cpp:942
 /// Finds declarations locations that a given source location refers to.
 class DeclarationLocationsFinder : public index::IndexDataConsumer {
   std::vector<Location> DeclarationLocations;
----------------
I think this class should be exactly the same as the code hover patch, so changes from there and the comments on that patch should apply here too.


================
Comment at: clangd/ClangdUnit.cpp:944
   std::vector<Location> DeclarationLocations;
+  std::vector<const Decl *> DeclarationDecls;
+  std::vector<const MacroInfo *> DeclarationMacroInfos;
----------------
just Decls?


================
Comment at: clangd/ClangdUnit.cpp:945
+  std::vector<const Decl *> DeclarationDecls;
+  std::vector<const MacroInfo *> DeclarationMacroInfos;
+  std::vector<DocumentHighlightKind> Kinds;
----------------
just MacroInfos?


================
Comment at: clangd/ClangdUnit.cpp:946
+  std::vector<const MacroInfo *> DeclarationMacroInfos;
+  std::vector<DocumentHighlightKind> Kinds;
   const SourceLocation &SearchedLocation;
----------------
remove, see comment below.


================
Comment at: clangd/ClangdUnit.cpp:961
+       std::sort(DeclarationDecls.begin(), DeclarationDecls.end());
+       auto last =
+           std::unique(DeclarationDecls.begin(), DeclarationDecls.end());
----------------
Last


================
Comment at: clangd/ClangdUnit.cpp:969
+    // Don't keep the same location multiple times.
+    // This can happen when nodes in the AST are visited twice.
+    std::sort(DeclarationMacroInfos.begin(), DeclarationMacroInfos.end());
----------------
This comment needs to be updated (like the code hover patch)


================
Comment at: clangd/ClangdUnit.cpp:971
+    std::sort(DeclarationMacroInfos.begin(), DeclarationMacroInfos.end());
+    auto last =
+        std::unique(DeclarationMacroInfos.begin(), DeclarationMacroInfos.end());
----------------
Last


================
Comment at: clangd/ClangdUnit.cpp:992
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
+
     if (isSearchedLocation(FID, Offset)) {
----------------
extra line


================
Comment at: clangd/ClangdUnit.cpp:996
+      DeclarationDecls.push_back(D);
+      DocumentHighlightKind Kind;
+      switch (Roles) {
----------------
I think you need to do this in DocumentHighlightsFinder not here. Each occurrence can have its kind, not just the user selected location.


================
Comment at: clangd/ClangdUnit.cpp:1013
 
+  std::vector<const Decl *> getDeclarationDecls() { return DeclarationDecls; };
+  std::vector<const MacroInfo *> getDeclarationMacroInfos() { return DeclarationMacroInfos; };
----------------
I don't think you need any of those getters. Only use the "takes"


================
Comment at: clangd/ClangdUnit.cpp:1079
 
+/// Finds document highlights that a given FileID and file offset refers to.
+class DocumentHighlightsFinder : public index::IndexDataConsumer {
----------------
It's not "a given FileID and file", the finder is given a list of Decls


================
Comment at: clangd/ClangdUnit.cpp:1122
 
+Location clangd::addDeclarationLocation(ParsedAST &AST, SourceRange SR) {
+  const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
----------------
This is duplicated code with getDeclarationLocation. It also doesn't use tryGetRealPathName
I also don't think this is called??


================
Comment at: clangd/ClangdUnit.cpp:1165
 
+Location clangd::getDeclarationLocation(ParsedAST &AST,
+                                        const SourceRange &ValSourceRange) {
----------------
I don't think this is called?


================
Comment at: clangd/ClangdUnit.cpp:1191
+
+DocumentHighlight clangd::addHighlightLocation(ParsedAST &AST, SourceRange SR,
+                                               DocumentHighlightKind Kind) {
----------------
This doesn't add  or have a location in its signature so I think it should be named differently. 
DocumentHighligh getDocumentHighlight(Decl*) ? Or addDocumentHighlight(Decl*) if you move it in DocumentHighlightsFinder.


================
Comment at: clangd/ClangdUnit.cpp:1232
+
+  std::vector<const Decl *> TempDecls = DeclLocationsFinder->getDeclarationDecls();
+  std::vector<DocumentHighlightKind> TempKinds =
----------------
call takeDecls. TempDecls -> SelectedDecls?


================
Comment at: clangd/ClangdUnit.cpp:1233
+  std::vector<const Decl *> TempDecls = DeclLocationsFinder->getDeclarationDecls();
+  std::vector<DocumentHighlightKind> TempKinds =
+      DeclLocationsFinder->getKinds();
----------------
remove? see comment below


================
Comment at: clangd/ClangdUnit.cpp:1243
+
+  std::vector<DocumentHighlight> HighlightLocations;
+
----------------
DocumentHighlights. They are not Locations (so they are not confused with the struct in Protocol)


================
Comment at: clangd/ClangdUnit.cpp:1245
+
+  for (unsigned I = 0; I < DocHighlightsFinder->getSourceRanges().size(); I++) {
+    HighlightLocations.push_back(
----------------
replace all this code (1242-1265) by moving it in DocumentHighlightsFinder? then call takeHighlights.


================
Comment at: clangd/ClangdUnit.cpp:1251
+
+  for (unsigned I = 0; I < DeclLocationsFinder->getDeclarationMacroInfos().size(); I++) {
+    HighlightLocations.push_back(addHighlightLocation(
----------------
I think we should remove macro handling for now, it doesn't highlight occurrences anyway.


================
Comment at: clangd/ClangdUnit.cpp:1265
+  HighlightLocations.erase(last, HighlightLocations.end());
+  return std::move(HighlightLocations);
+}
----------------
remove std::move, returning such local object will use copy elision


================
Comment at: clangd/ClangdUnit.h:318
 
+Location addDeclarationLocation(ParsedAST &AST, SourceRange SR);
+
----------------
remove?


================
Comment at: clangd/Protocol.h:637
+  DocumentHighlightKind kind = DocumentHighlightKind::Text;
+  int test = 3;
+
----------------
remove


================
Comment at: clangd/Protocol.h:639
+
+  friend bool operator<(const DocumentHighlight &LHS,
+                        const DocumentHighlight &RHS) {
----------------
needed?


================
Comment at: clangd/ProtocolHandlers.cpp:49
 };
-
 } // namespace
----------------
revert change


================
Comment at: clangd/ProtocolHandlers.h:34
   virtual ~ProtocolCallbacks() = default;
-
   virtual void onInitialize(Ctx C, InitializeParams &Params) = 0;
----------------
revert change


https://reviews.llvm.org/D38425