This revision was automatically updated to reflect the committed changes.
VitaNuo marked 3 inline comments as done.
Closed by commit rG2bececb8bed1: [clangd] Add provider info on symbol hover.
(authored by VitaNuo).
Changed prior to commit:
https://reviews.llvm.org/D144976?vs=507034&id=507764#t
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
Thanks, this looks great! Please fix the code style of the unit-test (see my
other comment) before landing it.
Comment at: clang-tools-extra/clangd/Hover.cpp:
+ //
VitaNuo added a comment.
Thanks for the review!
Comment at: clang-tools-extra/clangd/Hover.cpp:1099
+ const SourceManager &SM = AST.getSourceManager();
+ llvm::SmallVector Headers =
+ include_cleaner::headersForSymbol(Sym, SM, AST.getPragmaIncludes());
h
VitaNuo updated this revision to Diff 507034.
VitaNuo marked 10 inline comments as done.
VitaNuo added a comment.
Address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144976/new/
https://reviews.llvm.org/D144976
Files:
clang-to
hokein added a comment.
Thanks, this looks great, some more comments (most are nits)
Comment at: clang-tools-extra/clangd/Hover.cpp:1099
+ const SourceManager &SM = AST.getSourceManager();
+ llvm::SmallVector Headers =
+ include_cleaner::headersForSymbol(Sym, SM, AST.get
VitaNuo added a comment.
Thanks for the comments and the discussions!
Comment at: clang-tools-extra/clangd/Hover.cpp:1118
+
+ for (const auto &H : Headers) {
+if (H.kind() == include_cleaner::Header::Physical &&
hokein wrote:
> now the for-range loop doesn
VitaNuo updated this revision to Diff 506132.
VitaNuo marked 15 inline comments as done.
VitaNuo added a comment.
Address review comments, simplify test.
Take care of main file as provider handling:
- provider list [main-file,foo.h] and foo.h \#include'd - not show any
providers on hover.
- pro
hokein added a comment.
Thanks, left some comments on the unittest, I think we can make it simpler.
Comment at: clang-tools-extra/clangd/Hover.cpp:1107
+if (!Matches.empty()) {
+ // Pick the best-ranked included provider
+ Result = Matches[0]->quote();
---
VitaNuo added a comment.
thanks for the review!
Comment at: clang-tools-extra/clangd/Hover.cpp:1094
+void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo &HI,
+ std::optional UsedDecl,
hokein wrote:
> we can simplify the signatur
VitaNuo updated this revision to Diff 505460.
VitaNuo marked 4 inline comments as done.
VitaNuo added a comment.
Address the review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144976/new/
https://reviews.llvm.org/D144976
Files:
clang
hokein added inline comments.
Comment at: clang-tools-extra/clangd/Hover.cpp:1094
+void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo &HI,
+ std::optional UsedDecl,
we can simplify the signature like `(ParsedAST&, include_cleaner
VitaNuo marked an inline comment as done.
VitaNuo added a comment.
Thanks for the comments!
Comment at: clang-tools-extra/clangd/Hover.cpp:1099
+ trace::Span Tracer("Hover::maybeAddSymbolProviders");
+ include_cleaner::walkUsed(
+ AST.getLocalTopLevelDecls(), MacroRefere
VitaNuo updated this revision to Diff 505147.
VitaNuo added a comment.
Herald added a subscriber: ChuanqiXu.
Re-work the patch to use include_cleaner::headersForSymbol.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144976/new/
https://reviews.llvm.
hokein added inline comments.
Comment at: clang-tools-extra/clangd/Hover.cpp:1099
+ trace::Span Tracer("Hover::maybeAddSymbolProviders");
+ include_cleaner::walkUsed(
+ AST.getLocalTopLevelDecls(), MacroReferences, AST.getPragmaIncludes(),
It seems that th
VitaNuo updated this revision to Diff 504720.
VitaNuo added a comment.
Rebase to main.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144976/new/
https://reviews.llvm.org/D144976
Files:
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clang
VitaNuo added a comment.
Thank you for the comments! I've implemented the new version as per the design
doc, as well as did my best to address the review comments.
Comment at: clang-tools-extra/clangd/Hover.cpp:1093
+std::vector
+collectMacroReferences(ParsedAST &AST) {
+ con
VitaNuo updated this revision to Diff 504717.
VitaNuo marked 14 inline comments as done.
VitaNuo added a comment.
Implement the most recent version from the design document: always show a
single provider, whether directly included or not (as long as there is a
provider). Address review comments.
hokein added a comment.
Thanks, left a few comments on the implementation.
Comment at: clang-tools-extra/clangd/Hover.cpp:1091
+// FIXME(bakalova): Remove after merging https://reviews.llvm.org/D143496
+std::vector
I'm not sure how we can do it. https://revie
VitaNuo updated this revision to Diff 501162.
VitaNuo added a comment.
Replace start with arrow for optional access.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144976/new/
https://reviews.llvm.org/D144976
Files:
clang-tools-extra/clangd/Hover
VitaNuo created this revision.
VitaNuo added a reviewer: hokein.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
VitaNuo requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
Repositor
20 matches
Mail list logo
