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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
