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