This revision was automatically updated to reflect the committed changes.
Closed by commit rL325395: [clangd] Implement textDocument/hover (authored by
malaperle, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D35894
Files:
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE325395: [clangd] Implement textDocument/hover (authored by
malaperle, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D35894?vs=134708=134712#toc
Repository:
rCTE Clang Tools
malaperle accepted this revision.
malaperle added a comment.
This revision is now accepted and ready to land.
Works well and code looks good. There were only minor tweaks since the last
approval. I'll land this since there are some issues with Simon's svn account.
Repository:
rCTE Clang
malaperle updated this revision to Diff 134708.
malaperle added a comment.
Add a missing std::move, fix some formatting.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
malaperle updated this revision to Diff 134692.
malaperle added a comment.
Rebase.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Protocol.cpp
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
LGTM
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
simark updated this revision to Diff 134452.
simark added a comment.
Fix field names in XRefsTests.cpp
I realized I forgot to add some fixups in the previous version, the field names
in XRefsTests.cpp were not updated and therefore it did not build.
Repository:
rCTE Clang Tools Extra
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
Thanks for fixing all the NITs!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
simark updated this revision to Diff 134440.
simark added a comment.
Fix some nits
- Revert case change of struct fields in Protocol.h
- Change return formatting in onHover
- Use flush() + return Name in NamedDeclQualifiedName and TypeDeclToString
Repository:
rCTE Clang Tools Extra
simark added a comment.
In https://reviews.llvm.org/D35894#1008861, @ilya-biryukov wrote:
> Only the naming of fields left.
>
> Also, please make sure to run `git-clang-format` on the code before
> submitting to make sure it's formatted properly.
Ahh, I was running clang-format by hand,
ilya-biryukov added a comment.
Only the naming of fields left.
Also, please make sure to run `git-clang-format` on the code before submitting
to make sure it's formatted properly.
Comment at: clangd/ClangdLSPServer.cpp:331
+ if (!H) {
+
simark added a comment.
In https://reviews.llvm.org/D35894#1008593, @ilya-biryukov wrote:
> Just a few last remarks and this is good to go.
> Should I land it for you after the last comments are fixed?
I just received my commit access, so if you are ok with it, I would try to push
it once I
simark updated this revision to Diff 134437.
simark added a comment.
New version
- Rebase on master, change findHover to have a callback-based interface
- Fix nits from previous review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
Files:
simark added inline comments.
Comment at: clangd/XRefs.cpp:354
+
+ return Name;
+}
ilya-biryukov wrote:
> We should call `flush()` before returning `Name` here. The
> `raw_string_ostream` is buffered.
I replaced it with `Stream.str()`.
ilya-biryukov added a comment.
Just a few last remarks and this is good to go.
Should I land it for you after the last comments are fixed?
Comment at: clangd/XRefs.cpp:354
+
+ return Name;
+}
We should call `flush()` before returning `Name` here. The
simark added inline comments.
Comment at: unittests/clangd/XRefsTests.cpp:561
+
+EXPECT_EQ(H.contents.value, Test.expectedHover.str()) << Test.input;
+ }
Note that I used `.str()` here to make the output of failing tests readable and
useful. By default,
simark updated this revision to Diff 134306.
simark added a comment.
Generate Hover by pretty-printing the AST
This new version of the patch generates the hover by pretty-printing the AST.
The unit tests are updated accordingly. There are some inconsistencies in how
things are printed. For
ilya-biryukov added a comment.
In https://reviews.llvm.org/D35894#1006124, @simark wrote:
> Is there a way to get the macro name from the MacroInfo object? I couldn't
> find any, so the solution I'm considering is to make
> `DeclarationAndMacrosFinder::takeMacroInfos` return an
>
simark added a comment.
> I'm not aware of the code that pretty-prints macros. There's a code that
> dumps debug output, but it's not gonna help in that case.
> Alternatively, we could simply print the macro name and not print the
> definition. That's super-simple and is in line with hovers
ilya-biryukov added a comment.
In https://reviews.llvm.org/D35894#1003356, @simark wrote:
> In https://reviews.llvm.org/D35894#1003342, @simark wrote:
>
> > The only problem I see is that when hovering a function/struct name, it now
> > prints the whole function/struct definition. When talking
simark added a comment.
In https://reviews.llvm.org/D35894#1003342, @simark wrote:
> The only problem I see is that when hovering a function/struct name, it now
> prints the whole function/struct definition. When talking with @malaperle,
> he told me that you had discussed it before and we
simark added a comment.
The only problem I see is that when hovering a function name, it now prints the
whole function definition. When talking with @malaperle, he told me that you
had discussed it before and we should not have the definition in the hover,
just the prototype. Glancing
simark added a comment.
Another example where pretty-printing the AST gives better results:
int x, y = 5, z = 6;
Hover the `z` will now show `int z = 6`, before it would have shown `int x, y =
5, z = 6`. I think new version is better because it only shows the variable we
care about.
simark added inline comments.
Comment at: unittests/clangd/XRefsTests.cpp:231
+TEST(Hover, All) {
+
+ struct OneTest {
ilya-biryukov wrote:
> NIT: remove empty line at the start of the function
Done.
Comment at:
simark added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:325
+void ClangdLSPServer::onHover(TextDocumentPositionParams ) {
+
+ llvm::Expected H =
ilya-biryukov wrote:
> NIT: remove the empty line at the start of function
Done.
simark added a comment.
In https://reviews.llvm.org/D35894#1001875, @ilya-biryukov wrote:
> Thanks for picking this up!
> I have just one major comment related to how we generate the hover text.
>
> Current implementation tries to get the actual source code for every
> declaration and then
ilya-biryukov added a comment.
Thanks for picking this up!
I have just one major comment related to how we generate the hover text.
Current implementation tries to get the actual source code for every
declaration and then patch it up to look nice on in the output.
It is quite a large piece of
simark updated this revision to Diff 133367.
simark added a comment.
Herald added subscribers: ioeric, jkorous-apple.
New version
Here's a new version of the patch, quite reworked. I scaled down the scope a
little bit to make it simpler (see commit log). We'll add more features later
on.
sammccall added inline comments.
Comment at: clangd/ClangdUnit.cpp:941
createInvocationFromCommandLine(ArgStrs, CommandLineDiagsEngine,
VFS);
+ CI->LangOpts->CommentOpts.ParseAllComments = true;
// createInvocationFromCommandLine sets DisableFree.
Nebiroth updated this revision to Diff 127413.
Nebiroth marked 3 inline comments as done.
Nebiroth added a comment.
findHover properly returns an error
Removed many cases of bad merging
Separated getHover in two functions
Minor indenting and NIT fixes
Repository:
rCTE Clang Tools
Nebiroth marked 21 inline comments as done.
Nebiroth added inline comments.
Comment at: clangd/ClangdServer.h:306
+ /// Run formatting for \p Rng inside \p File.
+ std::vector formatRange(PathRef File, Range Rng);
+ /// Run formatting for the whole \p File.
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:284
+void ClangdLSPServer::onCodeHover(Ctx C, TextDocumentPositionParams ) {
+
+ auto H = Server.findHover(C,
NIT: remove empty line
Comment at:
Nebiroth updated this revision to Diff 127131.
Nebiroth added a comment.
Rebase on master
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
malaperle requested changes to this revision.
malaperle added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: mgrang.
Needs to be rebased.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
malaperle accepted this revision.
malaperle added a comment.
This looks good to me. @ilya-biryukov Any objections?
I think we can do further iterations later to handle macros and other specific
cases (multiple Decls at the position, etc) . It's already very useful
functionality.
Repository:
Nebiroth updated this revision to Diff 125224.
Nebiroth added a comment.
Minor code cleanup
Merge with master
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
Nebiroth added inline comments.
Comment at: clangd/Protocol.h:26
#include "llvm/ADT/Optional.h"
-#include
+#include "llvm/Support/YAMLParser.h"
#include
malaperle wrote:
> Nebiroth wrote:
> > malaperle wrote:
> > > revert this change?
> > #include is not
malaperle added inline comments.
Comment at: clangd/Protocol.h:26
#include "llvm/ADT/Optional.h"
-#include
+#include "llvm/Support/YAMLParser.h"
#include
Nebiroth wrote:
> malaperle wrote:
> > revert this change?
> #include is not needed.
I meant removing
Nebiroth marked 9 inline comments as done.
Nebiroth added inline comments.
Comment at: clangd/Protocol.h:26
#include "llvm/ADT/Optional.h"
-#include
+#include "llvm/Support/YAMLParser.h"
#include
malaperle wrote:
> revert this change?
#include is not
Nebiroth updated this revision to Diff 124945.
Nebiroth marked an inline comment as done.
Nebiroth added a comment.
Simplified getHover() function
Proper usage of ErrorOr to return errors
Given range for Hover struct now only applies to the open file
Fixed crash on MacroExpansion
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:1117
+
+ if (!DeclVector.empty()) {
+const Decl *LocationDecl = DeclVector[0];
malaperle wrote:
> malaperle wrote:
> > malaperle wrote:
> > > I think we need to simplify this part a bit.
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:1071
+Location getDeclarationLocation(ParsedAST ,
+const SourceRange ) {
llvm::ErrorOr
Comment at: clangd/ClangdUnit.cpp:1075
+ const
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/Protocol.cpp:498
+// Default Hover values
+Hover H;
+return json::obj{
remove, we have to return the
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:1117
+
+ if (!DeclVector.empty()) {
+const Decl *LocationDecl = DeclVector[0];
malaperle wrote:
> I think we need to simplify this part a bit. I'll try to find a way. Feel
> free to wait
Nebiroth updated this revision to Diff 124833.
Nebiroth added a comment.
Herald added a subscriber: klimek.
Invalid FileEntries now return llvm:ErrorOr
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:1029
+ SourceLocation LocStart = ValSourceRange.getBegin();
+ SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(),
0,
+ SourceMgr,
malaperle added inline comments.
Comment at: clangd/ClangdUnit.h:320
+
+StringRef getDataBufferFromSourceRange(ParsedAST , SourceRange SR,
+ Location L);
I think this can be only in the source file, in a an anonymous
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:1135
+RawComment *RC =
+AST.getASTContext().getRawCommentForDeclNoCache(LocationDecl);
+if (RC) {
Not sure why but I don't get the comments when I hover on "push_back"
malaperle added inline comments.
Comment at: clangd/Protocol.h:453
+struct MarkedString {
+ /**
+ * MarkedString can be used to render human readable text. It is either a
The doc should use /// like the others
https://reviews.llvm.org/D35894
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:1173
+H.range = L.range;
+Ref = getDataBufferFromSourceRange(AST, SR, L);
+ }
malaperle wrote:
> malaperle wrote:
> > I get the same crash as I mentioned before if I hover on
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:1173
+H.range = L.range;
+Ref = getDataBufferFromSourceRange(AST, SR, L);
+ }
malaperle wrote:
> I get the same crash as I mentioned before if I hover on the class in
>
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdLSPServer.cpp:245
+
+ C.reply(Hover::unparse(H->Value));
+}
we need to check the "Expected" here, so
if
malaperle added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:230
+ std::vector LocationVector;
+ auto test = Items->Value;
+
remove
Comment at: clangd/ClangdLSPServer.cpp:236
+
+ C.reply(json::ary(LocationVector));
}
Nebiroth updated this revision to Diff 123958.
Nebiroth marked 10 inline comments as done.
Nebiroth added a comment.
Removed accidentally included files
Fixed some coding standard issues
Removed getDeclarationLocation declaration from header file
Replaced getFunctionComments with clang
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
I haven't looked into the `getHover` method's body yet, but here are some
comments about other parts.
Good work, BTW, this looks close to being ready.
Nebiroth added a comment.
Forgot to mention this last patch also added support for displaying function
comments above the function definition displayed.
https://reviews.llvm.org/D35894
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Nebiroth updated this revision to Diff 123662.
Nebiroth marked 10 inline comments as done.
Nebiroth added a comment.
Removed all std::pair objects
Fixed and updated all tests
findHover doesn't call findDefinitions anymore
DeclarationLocationsFinder fills two Decl and MacroInfos vectors instead of
Nebiroth marked 6 inline comments as done.
Nebiroth added inline comments.
Comment at: clangd/ClangdUnit.cpp:981
+}
+
if (isSearchedLocation(FID, Offset)) {
malaperle wrote:
> I think we need to do a check that the computed SourceRange is valid
>
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
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
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdLSPServer.cpp:205
+
+ if (!(H.contents[0].codeBlockLanguage == "" &&
+H.contents[0].markdownString == "" &&
Nebiroth added a comment.
I also have a quick patch that supports displaying comments preceding the
declaration of a function. Once the review comments have been addressed for
this revision I will submit it.
https://reviews.llvm.org/D35894
___
Nebiroth updated this revision to Diff 120894.
Nebiroth added a comment.
Code hover now displays declaration of symbol instead of source code by default.
Code hover now displays context information such as namespace and class name.
Array of MarkedString objects is now sent as response in JSON.
ilya-biryukov added a comment.
In https://reviews.llvm.org/D35894#903607, @malaperle wrote:
> Exactly! Although the without-language part, maybe it'll be "scope"
> information first. Maybe documentation/comments in a second patch.
Sure, whatever works/looks best.
In
Nebiroth added a comment.
In https://reviews.llvm.org/D35894#903594, @ilya-biryukov wrote:
> In https://reviews.llvm.org/D35894#903552, @malaperle wrote:
>
> > I think he meant to have multiple sections in the hover, one C/C++ and one
> > not. But we noticed you can have an array of
malaperle added a comment.
In https://reviews.llvm.org/D35894#903594, @ilya-biryukov wrote:
> In https://reviews.llvm.org/D35894#903552, @malaperle wrote:
>
> > I think he meant to have multiple sections in the hover, one C/C++ and one
> > not. But we noticed you can have an array of
ilya-biryukov added a comment.
In https://reviews.llvm.org/D35894#903552, @malaperle wrote:
> I think he meant to have multiple sections in the hover, one C/C++ and one
> not. But we noticed you can have an array of MarkedString in Hover so it
> should be fine.
I totally misunderstood the
malaperle added a comment.
In https://reviews.llvm.org/D35894#903542, @ilya-biryukov wrote:
> In https://reviews.llvm.org/D35894#895023, @Nebiroth wrote:
>
> > I would like some feedback/suggestions on whether it's possible to "append"
> > information to a MarkedString to be displayed on
ilya-biryukov added a comment.
In https://reviews.llvm.org/D35894#895023, @Nebiroth wrote:
> I would like some feedback/suggestions on whether it's possible to "append"
> information to a MarkedString to be displayed on client?
You mean append **after** returning `MarkedString` to the client?
Nebiroth added a comment.
Bumping this.
I've worked on a patch for this feature that currently supports showing the
declaration of whatever is being hovered on instead of the raw source code. It
also has basic support to distinguish declarations in types ( class/struct,
namespace, global
malaperle added a comment.
In https://reviews.llvm.org/D35894#830178, @ilya-biryukov wrote:
> In https://reviews.llvm.org/D35894#829190, @malaperle wrote:
>
> > @Nebiroth I think it's OK to put this on hold until we make the "semantic"
> > hover and figure out how to have both. From our
ilya-biryukov added a comment.
In https://reviews.llvm.org/D35894#829190, @malaperle wrote:
> @Nebiroth I think it's OK to put this on hold until we make the "semantic"
> hover and figure out how to have both. From our perspective, this is going
> beyond parity of what we had before but it's
malaperle added a comment.
@Nebiroth I think it's OK to put this on hold until we make the "semantic"
hover and figure out how to have both. From our perspective, this is going
beyond parity of what we had before but it's seems like the right thing to do.
https://reviews.llvm.org/D35894
malaperle added a comment.
In https://reviews.llvm.org/D35894#829109, @ilya-biryukov wrote:
> > I think all of those would be great. Our objective is to bring basic but
> > correct features that will put us close to parity with Eclipse CDT, so that
> > our users can transition. In CDT only the
ilya-biryukov added a comment.
> I think all of those would be great. Our objective is to bring basic but
> correct features that will put us close to parity with Eclipse CDT, so that
> our users can transition. In CDT only the "raw" source is shown, similar to
> this patch (except in CDT it
malaperle added a comment.
In https://reviews.llvm.org/D35894#828702, @ilya-biryukov wrote:
> I just wanted to take a step back and discuss what we want to get from code
> hover in the first place.
>
> I would expect a typical code hover to be a bit more involved and include:
>
> - "kind" of a
ilya-biryukov added a reviewer: ilya-biryukov.
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
I just wanted to take a step back and discuss what we want to get from code
hover in the first place.
I would expect a typical code hover to be a bit more involved and
malaperle requested changes to this revision.
malaperle added a comment.
This revision now requires changes to proceed.
Can you also update your code with the latest SVN trunk? The patch does not
apply cleanly anymore.
Comment at: clangd/ClangdServer.cpp:11
#include
Nebiroth updated this revision to Diff 108997.
Nebiroth added a comment.
Implemention of Code Hover as described in LSP definition.
Removed unintentional changes to formatting.
Removed file id field in Location struct.
https://reviews.llvm.org/D35894
Files:
clangd/ClangdLSPServer.cpp
Nebiroth added a comment.
I had missed a typo in the code, should be fixed and compiling properly now.
https://reviews.llvm.org/D35894
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Nebiroth updated this revision to Diff 108979.
Nebiroth marked 19 inline comments as done.
Nebiroth added a comment.
Implemention of Code Hover as described in LSP definition.
Removed unintentional changes to formatting.
Removed file id field in Location struct.
https://reviews.llvm.org/D35894
81 matches
Mail list logo
