This revision was automatically updated to reflect the committed changes.
Closed by commit rL320474: [clangd] Document highlights for clangd (authored by
ibiryukov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D38425?vs=126371&id=126529#toc
Repository:
rL LLVM
https://r
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM minor a slight tweak to fix compilation. I'll land this today.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
_
Nebiroth added a comment.
@ilya-biryukov Need someone to land this.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm
Nebiroth updated this revision to Diff 126371.
Nebiroth marked 3 inline comments as done.
Nebiroth added a comment.
Herald added a subscriber: mgrang.
Merged with latest llvm/clang
Minor code cleanup
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSP
ilya-biryukov added a comment.
Overall looks good, just a few minor comments.
Let me know if need someone to land this patch for you after you fix those.
Comment at: clangd/ClangdUnit.cpp:249
+// Don't keep the same Macro info multiple times.
+// This can happen when no
malaperle accepted this revision.
malaperle added a subscriber: sammccall.
malaperle added a comment.
This looks good to me. @ilya-biryukov, @sammccall Any objections?
I think we can do further iterations later to handle macros and other specific
cases (multiple Decls at the position, etc) . It'
Nebiroth updated this revision to Diff 125619.
Nebiroth added a comment.
Added static_cast when unparsing
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clan
malaperle requested changes to this revision.
malaperle added a comment.
This revision now requires changes to proceed.
Sorry I forgot to submit this additional comment in my last review pass :(
Comment at: clangd/Protocol.cpp:365
+ {"range", toJSON(DH.range)},
+ {"ki
Nebiroth updated this revision to Diff 125522.
Nebiroth added a comment.
Added error returns in findDocumentHighlights
Ran clang-format on ClangdUnit.h, .cpp and ClangdServer.cpp
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/C
malaperle added a comment.
Can you also reformat the code too with clang-format? I think there's a few
things in ClangdUnit.h/.cpp
Comment at: clangd/ClangdServer.cpp:521
+ std::shared_ptr Resources = Units.getFile(File);
+ assert(Resources && "Calling findDocumentHighlights
Nebiroth updated this revision to Diff 125346.
Nebiroth added a comment.
Minor code cleanup
Refactored findDocumentHighlights() to make tests pass when assertions are
enabled
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/Clan
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdServer.cpp:528
+ Value = llvm::make_error(
+ "invalid AST",
+ llvm::errc::invalid_argument);
-
Nebiroth updated this revision to Diff 125228.
Nebiroth added a comment.
Minor code cleanup
unparse and parse methods for JSON are updated
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.c
malaperle added inline comments.
Comment at: clangd/ClangdServer.cpp:526
+if (!AST)
+ llvm::make_error(
+ "invalid AST",
missing return?
I get a warning that looks legit:
./tools/clang/tools/extra/clangd/ClangdServer.cpp:526:7: warning: igno
malaperle requested changes to this revision.
malaperle added a comment.
This revision now requires changes to proceed.
very minor comments
Comment at: clangd/ClangdServer.h:288
+ /// Get document highlights for a symbol hovered on.
+ llvm::Expected>>
+ findDocumentHighlight
Nebiroth updated this revision to Diff 124951.
Nebiroth marked 6 inline comments as done.
Nebiroth added a comment.
Minor code cleanup
getDeclarationLocation now returns llvm::Optional
operator< for DocumentHighlight struct now properly compares the kind field
Repository:
rCTE Clang Tool
ilya-biryukov requested changes to this revision.
ilya-biryukov added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdLSPServer.cpp:67
);
+
if (Params.rootUri && !Params.rootUri->file.empty())
malaperle wro
Nebiroth updated this revision to Diff 124834.
Nebiroth added a comment.
Fixed wrong content header making the test fail
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/Clangd
Nebiroth updated this revision to Diff 124810.
Nebiroth added a comment.
Added verification for llvm::Expected in onDocumentHighlight
Removed unused variable in ClangdUnit
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLS
malaperle added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:246
+
+ C.reply(json::ary(Highlights->Value));
+}
malaperle wrote:
> I get a test failure here because there is an assertion that the Expected<>
> needs to be checked. I can't really think
malaperle added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:246
+
+ C.reply(json::ary(Highlights->Value));
+}
I get a test failure here because there is an assertion that the Expected<>
needs to be checked. I can't really think of any failure case r
Nebiroth updated this revision to Diff 124458.
Nebiroth marked 3 inline comments as done.
Nebiroth added a comment.
Minor code cleanup
Fixed highlights sometimes obtaining one too many characters inside range
Updated test
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38425
Fil
malaperle requested changes to this revision.
malaperle added a comment.
This revision now requires changes to proceed.
I tested the patch and it works quite well! I think those are the last comments
from me. Sorry, I should have bundled them together a bit more :(
Comment at:
Nebiroth updated this revision to Diff 124391.
Nebiroth marked an inline comment as done.
Nebiroth added a comment.
Herald added a subscriber: klimek.
Removed temporary test file
Updated test to account for read-access symbol verification
Repository:
rCTE Clang Tools Extra
https://reviews.llv
malaperle requested changes to this revision.
malaperle added a comment.
This revision now requires changes to proceed.
There were some things I missed, sorry about that!
Comment at: clangd/main.cpp:1
+#define MACRO 1
+namespace ns1 {
This files needs to be rem
Nebiroth updated this revision to Diff 124238.
Nebiroth marked 3 inline comments as done.
Nebiroth added a comment.
Fixed test checking for values from an incorrect bit shift
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
Comment at: test/clangd/documenthighlight.test:1
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF l
Nebiroth updated this revision to Diff 124230.
Nebiroth marked 6 inline comments as done.
Nebiroth added a comment.
Fixed a few outstanding issues that were reported as completed
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdLSPServer.cpp:242
+
+ auto Items = Server
+ .findDocumentHighlights(Params.textDocument.uri.file,
Nebiroth updated this revision to Diff 124224.
Nebiroth marked an inline comment as done.
Nebiroth added a comment.
Getting DocumentHighlightKind is now done in DocumentHighlightsFinder
Removed duplicated and unused code
Refactored method and variable names
https://reviews.llvm.org/D38425
Files
Nebiroth marked 31 inline comments as done.
Nebiroth added inline comments.
Comment at: clangd/ClangdServer.h:291
+ /// Get document highlights for a symbol hovered on.
+ Tagged> findDocumentHighlights(PathRef File,
+
malaperle added a comment.
In https://reviews.llvm.org/D38425#922408, @ioeric wrote:
> Drive-by comment: in general, have you considered reusing the existing
> declarations and occurrences finding functionalities in clang-rename? AFAIK,
> it deals with templates and macros pretty well.
>
> o
>
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:997
+ DocumentHighlightKind Kind;
+ switch (Roles) {
+ case (unsigned)index::SymbolRole::Read:
With this code, I always get "text" kind. It's because index::SymbolRoleSet is
a bitf
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:1245
+
+ for (unsigned I = 0; I < DocHighlightsFinder->getSourceRanges().size(); I++)
{
+HighlightLocations.push_back(
malaperle wrote:
> replace all this code (1242-1265) by moving it in Do
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdLSPServer.cpp:67
);
+
if (Params.rootUri && !Params.rootUri->file.empty())
extra line
Nebiroth updated this revision to Diff 123848.
Nebiroth added a comment.
Removed some commented lines and temporary code
Streamlined and removed some code that overlaps/conflicts with code hover patch
so it's easier to merge (patch https://reviews.llvm.org/D35894)
https://reviews.llvm.org/D3842
malaperle added a comment.
Hi William, can you quickly go over the diff before I do a review? There are a
few more-obvious things that can be addressed, i.e. commented lines, extra
new/deleted lines, temporary code, lower case variable names, etc.
https://reviews.llvm.org/D38425
___
Nebiroth added a comment.
This updated patch still does not handle highlighting macro references
correctly. I will make another patch at a later time for this issue.
In https://reviews.llvm.org/D38425#922408, @ioeric wrote:
> Drive-by comment: in general, have you considered reusing the existin
ioeric added a comment.
Drive-by comment: in general, have you considered reusing the existing
declarations and occurrences finding functionalities in clang-rename? AFAIK, it
deals with templates and macros pretty well.
o
https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/R
Nebiroth updated this revision to Diff 122529.
Nebiroth added a comment.
Decls and MacroInfos vectors are now private and passed by reference instead of
copied.
DeclarationLocationsFinder does not store Locations anymore, instead the vector
is filled in their respective methods in ClangdUnit.cpp
Nebiroth updated this revision to Diff 122528.
Nebiroth added a comment.
- Decls and MacroInfos vectors are now private and passed by reference instead
of copied.
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/C
ilya-biryukov added a comment.
> Another note: current implementation does not seem to handle macros at all
> (it will only highlight definition of a macro, not its usages).
Current implementation still doesn't highlight macro references, only
definitions. I'd either disable `documentHighlight`
Nebiroth updated this revision to Diff 118838.
Nebiroth marked an inline comment as done.
Nebiroth added a comment.
Addressed review comments.
Fixed some tests not having updated providers.
Removed TargetDeclarationFinder for less code reuse.
DocumentHighlight struct is now unparsed correctly.
h
Nebiroth marked an inline comment as done.
Nebiroth added inline comments.
Comment at: clangd/ClangdUnit.cpp:784
+/// Finds declarations locations that a given source location refers to.
+class TargetDeclarationFinder : public index::IndexDataConsumer {
+ std::vector Declaration
Nebiroth marked 3 inline comments as done.
Nebiroth added inline comments.
Comment at: clangd/ClangdUnit.cpp:1017
+
+ auto DeclLocationsFinder = std::make_shared(
+ llvm::errs(), SourceLocationBeg, AST.getASTContext(),
ilya-biryukov wrote:
> I wonder if we
Nebiroth updated this revision to Diff 118482.
Nebiroth added a comment.
Rebased on master.
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Pro
ilya-biryukov added a comment.
Sorry for the delay.
The patch does not apply cleanly on top of the current head. Mostly conflicts
with `switchHeaderSource` for me.
Could you please rebase your change with head and resubmit it?
Another note: current implementation does not seem to handle macros
Nebiroth updated this revision to Diff 117351.
Nebiroth marked 3 inline comments as done.
Nebiroth added a comment.
Addressed initial comments.
Formatted ClangdUnit.h
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/C
ilya-biryukov added a comment.
Thanks for the patch, I'll take a closer look a bit later.
But just wanted to post one very important comment right away.
Comment at: clangd/ClangdUnit.h:268
+std::vector findDocumentHighlights(ParsedAST &AST, Position
Pos,
+
malaperle added a comment.
Just a few quick comments.
Comment at: clangd/ClangdServer.cpp:295
+ assert(FileContents.Draft &&
+ "findDefinitions is called for non-added document");
+
findDocumentHighlights?
Comment at: clangd/ClangdSe
Nebiroth created this revision.
Implementation of Document Highlights Request as described in LSP.
https://reviews.llvm.org/D38425
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Protocol.cpp
clangd/P
51 matches
Mail list logo
