[PATCH] D38425: [clangd] Document highlights for clangd

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=126529#toc Repository: rL LLVM

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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:

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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)}, +

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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:

[PATCH] D38425: [clangd] Document highlights for clangd

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> +

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for 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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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, +

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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)

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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.

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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

[PATCH] D38425: [clangd] Document highlights for clangd

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 , Position Pos, +

[PATCH] D38425: [clangd] Document highlights for clangd

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:

[PATCH] D38425: [clangd] Document highlights for clangd

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