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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for late response, was on vacation.
+1 to all @malaperle 's comments.

Here are some extra quick comments, will take a closer look later.



================
Comment at: clangd/ClangdServer.cpp:11
 #include "ClangdServer.h"
+#include "Protocol.h"
 #include "clang/Format/Format.h"
----------------
malaperle wrote:
> I don't know if the intention was to keep the protocol completely out of the ClangdServer class. Maybe someone else can clarify. But either way, I see that ClangdUnit does include "Protocol.h" so it's probably OK for now.
We don't want dependencies on JSON parsing/unparsing and the protocol itself. We reuse some of the LSP class definitions to avoid code duplication, though.

This include is redundant, though, as `ClangdServer.h` already include `Protocol.h`. 


================
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);
----------------
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.


https://reviews.llvm.org/D35894