[PATCH] D35894: [clangd] Code hover for Clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.

I haven't looked into the `getHover` method's body yet, but here are some comments about other parts.
Good work, BTW, this looks close to being ready.



================
Comment at: clangd/ClangdServer.cpp:511
+Tagged<Hover> ClangdServer::findHover(PathRef File, Position Pos) {
+
+  //Default Hover values
----------------
NIT: could you remove the empty newlines at the start of the methods?


================
Comment at: clangd/ClangdServer.cpp:514
+  std::vector<MarkedString> EmptyVector;
+  Position BeginPosition = {0,0};
+  Position EndPosition = {0,0};
----------------
Let's follow error-reporting pattern used in other `ClangdServer` features.
We should return `llvm::Expected` and do `Ctx.replyError` on error return values.


================
Comment at: clangd/ClangdUnit.cpp:959
+    std::sort(DeclarationDecls.begin(), DeclarationDecls.end());
+    auto last = std::unique(DeclarationDecls.begin(), DeclarationDecls.end());
+    DeclarationDecls.erase(last, DeclarationDecls.end());
----------------
NIT: local vars are `UpperCamelCase`. (Not introduced by this commit, but probably a good place to fix the naming anyway).


================
Comment at: clangd/ClangdUnit.cpp:967
+    // This can happen when nodes in the AST are visited twice.
+    std::sort(DeclarationMacroInfs.begin(), DeclarationMacroInfs.end());
     auto last =
----------------
NIT: local vars are `UpperCamelCase`. (Not introduced by this commit, but probably a good place to fix the naming anyway).


================
Comment at: clangd/ClangdUnit.cpp:1100
+
+  const SourceManager &SM = AST.getASTContext().getSourceManager();
+  const LangOptions &LangOpts = AST.getASTContext().getLangOpts();
----------------
I think this functionality is already implemented in clang. Could you use `ASTContext::getRawCommentForAnyRedecl`?


================
Comment at: clangd/ClangdUnit.cpp:1199
+      } else if (dyn_cast<TypedefDecl>(LocationDecl)) {
+        // TODO: find a way to get the hover for the type that is being aliased
+        SR = LocationDecl->getSourceRange();
----------------
NIT: we use `FIXME`


================
Comment at: clangd/ClangdUnit.h:323
+
+Location getDeclarationLocation(ParsedAST &AST, const SourceRange &ValSourceRange);
+
----------------
Could we put `getDeclarationLocation` and `getFunctionComments` in the cpp file instead? They're just implementation detail of `findDefinitions`/`getHover`, right?


================
Comment at: clangd/MyClass.cpp:1
+#include "MyClass.h"
+
----------------
Remove it.


================
Comment at: clangd/Protocol.h:26
 #include "llvm/ADT/Optional.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/Optional.h"
----------------
We don't need `SourceLocation`  here


================
Comment at: clangd/Protocol.h:27
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/YAMLParser.h"
----------------
This include is redundant


https://reviews.llvm.org/D35894