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

[William Enright via Phabricator via cfe-commits]

Nebiroth marked 25 inline comments as done.
Nebiroth added inline comments.


================
Comment at: clangd/ClangdServer.cpp:360
+
+  auto FileContents = DraftMgr.getDraft(File);
+  assert(FileContents.Draft &&
----------------
malaperle wrote:
> Why are you adding this? Is this coming from another commit?
> It looks like it makes tests fail:
> <-- {"jsonrpc":"2.0","id":2,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///doesnotexist.cpp"},"position":{"line":4,"character":7}}}
> clangd: ../tools/clang/tools/extra/clangd/ClangdServer.cpp:362: llvm::Expected<clang::clangd::Tagged<std::vector<std::pair<clang::clangd::Location, const clang::Decl*> > > > clang::clangd::ClangdServer::findDefinitions(clang::clangd::PathRef, clang::clangd::Position): Assertion `FileContents.Draft && "findDefinitions is called for non-added document"' failed.
> /home/emalape/git/llvm/tools/clang/tools/extra/test/clangd/definitions.test:169:10: error: expected string not found in input
> 
Accidental addition.


================
Comment at: clangd/ClangdServer.h:281
   /// Get definition of symbol at a specified \p Line and \p Column in \p File.
-  llvm::Expected<Tagged<std::vector<Location>>> findDefinitions(PathRef File,
+  llvm::Expected<Tagged<std::vector<std::pair<Location, const Decl*>>>> findDefinitions(PathRef File,
                                                                 Position Pos);
----------------
ilya-biryukov wrote:
> malaperle wrote:
> > Location can be deduced from Decl*. I don't think this should be a pair. I also think we need a more general finder that just gets either the Decl* or MacroDef* at the "cursor". I think CXCursor does something similar in that it stores either Decl* or MacroDef*. I'm not sure it would be worth moving CXCursor from libclang but perhaps something stripped down for Clangd would be OK. This new finder will be able to be reused by findDefinition, highlights, hover, references and more.
> > We can make one patch that refactors the existing code so that findDefinitions uses this new finder class.
> We should not return `Decl*` from `ClangdServer::findDefinitions`, it's an internal clang object (there are many other reasons why it's bad). 
> 
> It's ok the change the `findDefinitions` helper function inside `ClangdUnit.cpp`, though. Please change it instead.
> 
> As for the `CXCursor`, I think the simplest approach would be to return two vectors (`vector<Decl*>` and `vector<MacroDef*>`). I think clangd would eventually get its own `CXCursor`-like things, possible reusing code or wrapping CXCursor. But for now let's keep things simple.
We would have to handle MacroDef occurrences for this to work first. In other words, we would have to implement handleMacroDefOccurence in a similar way to handleDeclOccurence in DeclarationLocationsFinder. The document highlights feature suffers from the exact same problem. I'm planning on working on this feature in the near future.


================
Comment at: clangd/ClangdUnit.cpp:941
+
+    // Keep default value.
+    SourceRange SR = D->getSourceRange();
----------------
malaperle wrote:
> This code doesn't belong here. The hover feature and "find definition" do not need the same range. The hover basically wants everything up to the first left brace but "find definitions" wants everything up to the closing brace. So having this code here will make "find definition" less correct.
Agreed, I'll revert the code to normal here and move this patch's code to getHover().


https://reviews.llvm.org/D35894