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

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

malaperle added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:230
+  std::vector<Location> LocationVector;
+  auto test = Items->Value;
+
----------------
remove


================
Comment at: clangd/ClangdLSPServer.cpp:236
+
+  C.reply(json::ary(LocationVector));
 }
----------------
I think you can just do Items->Value here and not have LocationVector at all.


================
Comment at: clangd/ClangdLSPServer.cpp:251
+      Position{Params.position.line, Params.position.character});
+  if (!H)
+    return C.replyError(ErrorCode::InvalidParams,
----------------
I think we should return an empty Hover here and not an error. Since the empty Hover is already generated, this would mean simply removing those two lines.


================
Comment at: clangd/ClangdServer.cpp:11
 #include "ClangdServer.h"
+#include "Protocol.h"
 #include "clang/Format/Format.h"
----------------
Remove include?


================
Comment at: clangd/ClangdServer.cpp:432
 ClangdServer::findDefinitions(PathRef File, Position Pos) {
+
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);
----------------
revert


================
Comment at: clangd/ClangdServer.cpp:447
   });
+
   return make_tagged(std::move(Result), TaggedFS.Tag);
----------------
revert


================
Comment at: clangd/ClangdServer.cpp:517
+  Range DefaultRange = {BeginPosition, EndPosition};
+  Hover FinalHover = {EmptyVector, DefaultRange};
+
----------------
I think you remove all default values and just do 
Hover FinalHover;


================
Comment at: clangd/ClangdServer.cpp:527
+          return;
+        // Only show hover for the first found definition, we could potentially
+        // expand this in a later patch.
----------------
I don't think this comment belong here anymore?


================
Comment at: clangd/ClangdUnit.cpp:1187
+      } else if (auto ClassDecl = dyn_cast<TagDecl>(LocationDecl)) {
+        if (!ClassDecl->isInterface() && !ClassDecl->isUnion() &&
+            ClassDecl->isThisDeclarationADefinition())
----------------
what's wrong with interface and union?


================
Comment at: clangd/ClangdUnit.cpp:1246
+          Contexts.push_back(Ctx);
+          Ctx = Ctx->getParent();
+        }
----------------
Ctx is never used so you can remove lines 1231 to 1247


================
Comment at: clangd/ClangdUnit.cpp:942
 
 /// Finds declarations locations that a given source location refers to.
 class DeclarationLocationsFinder : public index::IndexDataConsumer {
----------------
This comment isn't accurate anymore.


================
Comment at: clangd/ClangdUnit.cpp:943
 /// Finds declarations locations that a given source location refers to.
 class DeclarationLocationsFinder : public index::IndexDataConsumer {
+  std::vector<const Decl *> DeclarationDecls;
----------------
I think this should be renamed, it doesn't find locations for declarations anymore. I don't know what to call it, hmmm DeclOrMacroAtLocationFinder? :)


================
Comment at: clangd/ClangdUnit.cpp:944
 class DeclarationLocationsFinder : public index::IndexDataConsumer {
-  std::vector<Location> DeclarationLocations;
+  std::vector<const Decl *> DeclarationDecls;
+  std::vector<const MacroInfo *> DeclarationMacroInfs;
----------------
just Decls?


================
Comment at: clangd/ClangdUnit.cpp:945
+  std::vector<const Decl *> DeclarationDecls;
+  std::vector<const MacroInfo *> DeclarationMacroInfs;
   const SourceLocation &SearchedLocation;
----------------
just MacroInfos?


================
Comment at: clangd/ClangdUnit.cpp:986
 
+  std::vector<const Decl *> getDeclarationDecls() { return DeclarationDecls; };
+  std::vector<const MacroInfo *> getMacroInfos() {
----------------
I don't think that getter is necessary, only use the take


================
Comment at: clangd/ClangdUnit.cpp:987
+  std::vector<const Decl *> getDeclarationDecls() { return DeclarationDecls; };
+  std::vector<const MacroInfo *> getMacroInfos() {
+    return DeclarationMacroInfs;
----------------
I don't think that getter is necessary, only use the take


================
Comment at: clangd/ClangdUnit.cpp:1044
+  Location L;
+  if (const FileEntry *F =
+          SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart))) {
----------------
It looks like this sometimes return null and we don't handle this well. Need to investigate this more though. See later comment.


================
Comment at: clangd/ClangdUnit.cpp:1086
+  for (auto Item : MacroInfos) {
+    SourceRange SR =
+        SourceRange(Item->getDefinitionLoc(), Item->getDefinitionEndLoc());
----------------
You can omit the "= SourceRange"


================
Comment at: clangd/ClangdUnit.cpp:1102
+  Range DefaultRange = {BeginPosition, EndPosition};
+  Hover H = {EmptyVector, DefaultRange};
+  unsigned Start, End;
----------------
you can remove the lines before, just
"Hover H;" is OK
Range is optional and the vector is already initialized to empty.


================
Comment at: clangd/ClangdUnit.cpp:1103
+  Hover H = {EmptyVector, DefaultRange};
+  unsigned Start, End;
+  StringRef Ref;
----------------
I think those should be declared where they are assigned. way below. They are just very short lived temporaries


================
Comment at: clangd/ClangdUnit.cpp:1125
+
+  if (!DeclLocationsFinder->getDeclarationDecls().empty()) {
+    const Decl *LocationDecl = DeclLocationsFinder->takeDecls()[0];
----------------
call takeDecls once and assign it to a variable? so no need for getDeclarationDecls


================
Comment at: clangd/ClangdUnit.cpp:1132
+        SourceLocation NewLocation;
+        if (AST.getASTContext().getRawCommentForDeclNoCache(LocationDecl)) {
+          NewLocation = AST.getASTContext()
----------------
I think we need to call this for pretty much any Decl, not juste FunctionDecl.
Also, assign to variable so that it's not called twice? It sounds like it could be costly.

I've also noticed weirdness when I hover on ClangdScheduler::addToFront for example, it shows
```
In clang::clangd::ClangdScheduler
/// Add \p Request to the start of the queue. \p Request will be run on a
  /// separate worker thread.
  /// \p Request is scheduled to be executed before all currently added
  /// requests.
  void addToFront(std::function<void()> Request)
```

Notice the missing space before the first comment line.

It also seems to be confused by templates, if I hover on the call Units.runOnUnit:
```
In clang::clangd::ClangdUnitStore
void runOnUnit(PathRef File, StringRef FileContents, StringRef ResourceDir,
                 GlobalCompilationDatabase &CDB,
                 std::shared_ptr<PCHContainerOperations> PCHs,
                 IntrusiveRefCntPtr<vfs::FileSystem> VFS, Func Action) {
```
But this should show the template line and a comment above it.


================
Comment at: clangd/ClangdUnit.cpp:1142
+                           FuncDecl->getBody()->getLocStart());
+      } else if (auto ClassDecl = dyn_cast<TagDecl>(LocationDecl)) {
+        if (!ClassDecl->isInterface() && !ClassDecl->isUnion() &&
----------------
ClassDecl is too specific, it can be a struct, union, etc.


================
Comment at: clangd/ClangdUnit.cpp:1143
+      } else if (auto ClassDecl = dyn_cast<TagDecl>(LocationDecl)) {
+        if (!ClassDecl->isInterface() && !ClassDecl->isUnion() &&
+            ClassDecl->isThisDeclarationADefinition())
----------------
Why can't this work with interface or unions?


================
Comment at: clangd/ClangdUnit.cpp:1155
+      } else if (dyn_cast<TypedefDecl>(LocationDecl)) {
+        // FIXME: find a way to get the hover for the type that is being aliased
+        SR = LocationDecl->getSourceRange();
----------------
I don't know if we want to get the type that is being aliased. It's useful to know that it's a typedef/alias. Maybe we can do a hover that you can "step through", similarly do the Macro Explorer in CDT (which we want to do in Clangd too.


================
Comment at: clangd/ClangdUnit.cpp:1168
+        const FileEntry *FE = SourceMgr.getFileManager().getFile(L.uri.file);
+        FileID FID = SourceMgr.translateFile(FE);
+        Ref = SourceMgr.getBufferData(FID);
----------------
I was able to get a crash here because FE was null. Indeed in getDeclarationLocation, it's possible that the Location.uri will not be set, whenever getFileEntryForID returns null. We need to know why and probably also change getDeclarationLocation to return an error. I'll investigate this a bit further though.
In case you want to try, I hover in llvm/lib/Option/Arg.cpp:114,  on Values.end()


================
Comment at: clangd/ClangdUnit.cpp:1170
+        Ref = SourceMgr.getBufferData(FID);
+        Start = SourceMgr.getFileOffset(SourceMgr.translateFileLineCol(
+            FE, L.range.start.line + 1, L.range.start.character + 1));
----------------
here is where I would put unsigned Start =
(see comment above)


================
Comment at: clangd/ClangdUnit.cpp:1172
+            FE, L.range.start.line + 1, L.range.start.character + 1));
+        End = SourceMgr.getFileOffset(SourceMgr.translateFileLineCol(
+            FE, L.range.end.line + 1, L.range.end.character + 1));
----------------
same


================
Comment at: clangd/ClangdUnit.cpp:1182
+        // trimming the extra "::"
+        PrintingPolicy WithScopePP(AST.getASTContext().getLangOpts());
+        std::string WithScopeBuf;
----------------
I think from here to line 1218 could be extracted to another function. Something like getScope(NamedDecl*) ?


================
Comment at: clangd/ClangdUnit.cpp:1187
+
+        // Get all contexts for current NamedDecl
+        const DeclContext *Ctx = NamedD->getDeclContext();
----------------
Ctx is never used so 1187-1203 can be removed


================
Comment at: clangd/ClangdUnit.cpp:1236
+                                   MacroInf->getDefinitionEndLoc());
+      if (SR.isValid()) {
+        Location L = getDeclarationLocation(AST, SR);
----------------
This whole block looks duplicated from above? Maybe extract to a function?


================
Comment at: clangd/Protocol.cpp:1019
+  if (H.range.hasValue())
+  {
+    return json::obj{
----------------
wrong brace placement


================
Comment at: clangd/Protocol.cpp:1031
+    Hover H = {EmptyVector, DefaultRange};
+    return json::obj{
+            {"contents", json::ary(H.contents)},
----------------
No need to initialize a Range, it's optional. So Here just
return json::obj{
            {"contents", json::ary(H.contents)}
};


================
Comment at: clangd/Protocol.h:482
+
+  Hover(std::vector<MarkedString> contents, Range r)
+      : contents(contents), range(r) {}
----------------
I don't think you need this constructor


================
Comment at: test/clangd/initialize-params-invalid.test:44
 # CHECK-NEXT:  }
+
 Content-Length: 44
----------------
revert


================
Comment at: test/clangd/initialize-params.test:8
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"file:///path/to/workspace","capabilities":{},"trace":"off"}}
+
 #      CHECK:  "id": 0,
----------------
revert


https://reviews.llvm.org/D35894