malaperle added a comment.
In https://reviews.llvm.org/D44882#1077516, @sammccall wrote:
> It makes sense, but @bkramer came up with some deep magic in
> https://reviews.llvm.org/rL330754 so I think we're actually good now.
Nice! Thanks @bkramer !
Repository:
rCTE Clang Tools Extra
sammccall added a subscriber: bkramer.
sammccall added a comment.
In https://reviews.llvm.org/D44882#1076927, @malaperle wrote:
> In https://reviews.llvm.org/D44882#1076864, @sammccall wrote:
>
> > So this fails if there's no standard library available without flags, which
> > is the case in
malaperle added a comment.
In https://reviews.llvm.org/D44882#1076864, @sammccall wrote:
> So this fails if there's no standard library available without flags, which
> is the case in google's test environment to ensure hermeticity :-(
>
> In the short-term, we've disabled the test internally -
sammccall added a comment.
In https://reviews.llvm.org/D44882#1066743, @malaperle wrote:
> In https://reviews.llvm.org/D44882#1065632, @sammccall wrote:
>
> > In https://reviews.llvm.org/D44882#1065631, @malaperle wrote:
> >
> > > In https://reviews.llvm.org/D44882#1065622, @sammccall wrote:
> >
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE330637: [clangd] Implementation of workspace/symbol
request (authored by malaperle, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D44882?vs=143220=143626#toc
Repository:
rCTE
malaperle updated this revision to Diff 143220.
malaperle added a comment.
Remove more includes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
malaperle added a comment.
Sorry for the embarrassing clean-ups I forgot!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
malaperle updated this revision to Diff 143219.
malaperle marked 5 inline comments as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
hokein added inline comments.
Herald added a subscriber: jkorous.
Comment at: clangd/FindSymbols.cpp:117
+}
+auto Path = URI::resolve(*Uri);
+if (!Path) {
The current URI scheme (`file`, `test`) works fine without `HintPath` because
they don't use
sammccall added a comment.
Sorry, missed this.
Just a few leftovers from removing the line/col conversion code.
Then ship it!
Comment at: clangd/ClangdServer.h:166
+ // FIXME: Remove this parameter when the index has line/col.
+ const
malaperle added a comment.
Any objections to the latest version?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
malaperle added a comment.
In https://reviews.llvm.org/D44882#1066883, @hokein wrote:
> Thanks, I have landed the patch today, you need to update your patch (I think
> it is mostly about removing the code of reading-file stuff).
I updated the patch using the new line/col from the index. This
malaperle updated this revision to Diff 142403.
malaperle marked 2 inline comments as done.
malaperle added a comment.
Address comments. Simplify with using line/col from index.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
sammccall accepted this revision.
sammccall added a comment.
Still LG
Comment at: clangd/ClangdLSPServer.cpp:113
+ auto KindVal = static_cast(Kind);
+ if (KindVal >= SymbolKindMin && KindVal <= SymbolKindMax)
+SupportedSymbolKinds.set(KindVal);
hokein added a comment.
In https://reviews.llvm.org/D44882#1065932, @malaperle wrote:
> In https://reviews.llvm.org/D44882#1065787, @hokein wrote:
>
> > @malaperle, what's your plan of this patch? Are you going to land it before
> > https://reviews.llvm.org/D45513? With the Line info in the
malaperle marked an inline comment as done.
malaperle added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:113
+ auto KindVal = static_cast(Kind);
+ if (KindVal >= SymbolKindMin && KindVal <= SymbolKindMax)
+SupportedSymbolKinds.set(KindVal);
malaperle updated this revision to Diff 142328.
malaperle marked an inline comment as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
malaperle added a comment.
In https://reviews.llvm.org/D44882#1065632, @sammccall wrote:
> In https://reviews.llvm.org/D44882#1065631, @malaperle wrote:
>
> > In https://reviews.llvm.org/D44882#1065622, @sammccall wrote:
> >
> > > Still LG, thanks!
> > > I'll look into the testing issue.
> >
>
malaperle added a comment.
In https://reviews.llvm.org/D44882#1065787, @hokein wrote:
> @malaperle, what's your plan of this patch? Are you going to land it before
> https://reviews.llvm.org/D45513? With the Line info in the index, this
> patch could be simplified.
I'll address the last
hokein added a comment.
@malaperle, what's your plan of this patch? Are you going to land it before
https://reviews.llvm.org/D45513? With the Line info in the index, this
patch could be simplified.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
sammccall added a comment.
In https://reviews.llvm.org/D44882#1065631, @malaperle wrote:
> In https://reviews.llvm.org/D44882#1065622, @sammccall wrote:
>
> > Still LG, thanks!
> > I'll look into the testing issue.
>
>
> I thought about it after... I think it was because I was trying to test
malaperle added a comment.
In https://reviews.llvm.org/D44882#1065622, @sammccall wrote:
> Still LG, thanks!
> I'll look into the testing issue.
I thought about it after... I think it was because I was trying to test with
std::unordered_map (to prevent multiple results) which needs
sammccall added a comment.
Still LG, thanks!
I'll look into the testing issue.
Comment at: clangd/ClangdLSPServer.cpp:113
+ auto KindVal = static_cast(Kind);
+ if (KindVal >= SymbolKindMin && KindVal <= SymbolKindMax)
+SupportedSymbolKinds.set(KindVal);
malaperle added a comment.
In https://reviews.llvm.org/D44882#1064196, @sammccall wrote:
> (BTW @hokein has https://reviews.llvm.org/D45513 to add row/col to the index,
> which will allow all the file-reading stuff to be cleaned up, but no need to
> wait on that since it's all working now).
malaperle updated this revision to Diff 142112.
malaperle marked 2 inline comments as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
sammccall added a comment.
(BTW @hokein has https://reviews.llvm.org/D45513 to add row/col to the index,
which will allow all the file-reading stuff to be cleaned up, but no need to
wait on that since it's all working now).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Sorry about the delay Marc-André - I got stuck on "how to test ClangdLSPServer"
and then distracted!
But this looks great.
Comment at: clangd/ClangdLSPServer.cpp:101
malaperle added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:101
+ // All clients should support those.
+ for (SymKindUnderlyingType I = SymbolKindMin;
+ I <= static_cast(SymbolKind::Array); ++I)
I'd like to add some test to exercise the
malaperle updated this revision to Diff 141028.
malaperle marked 27 inline comments as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
malaperle added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:99
Params.capabilities.textDocument.completion.completionItem.snippetSupport;
+ if (Params.capabilities.workspace && Params.capabilities.workspace->symbol &&
+
hokein added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:258
+if (!Items)
+ return replyError(ErrorCode::InvalidParams,
+llvm::toString(Items.takeError()));
`InvalidParams` doesn't match all cases where
malaperle marked an inline comment as done.
malaperle added inline comments.
Comment at: clangd/FindSymbols.cpp:31
+
+ if (supportedSymbolKinds &&
+ std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(),
MaskRay wrote:
> MaskRay wrote:
> >
MaskRay added inline comments.
Comment at: clangd/FindSymbols.cpp:31
+
+ if (supportedSymbolKinds &&
+ std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(),
MaskRay wrote:
> malaperle wrote:
> > MaskRay wrote:
> > > This std::find loop adds
MaskRay added inline comments.
Comment at: clangd/FindSymbols.cpp:31
+
+ if (supportedSymbolKinds &&
+ std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(),
malaperle wrote:
> MaskRay wrote:
> > This std::find loop adds some overhead to
malaperle added inline comments.
Comment at: clangd/FindSymbols.cpp:31
+
+ if (supportedSymbolKinds &&
+ std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(),
MaskRay wrote:
> This std::find loop adds some overhead to each candidate... In
sammccall added a comment.
This is fantastic, really looking forward to using it.
I'm going to need to give up on vim at this rate, or do some serious work on
ycm.
Most significant comments are around the new functions in SourceLocation, and
whether we can keep the type-mapping restricted to
MaskRay added inline comments.
Comment at: clangd/FindSymbols.cpp:31
+
+ if (supportedSymbolKinds &&
+ std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(),
This std::find loop adds some overhead to each candidate... In my experience
the
malaperle added a comment.
I realized that using the draft store has limited improvement for now because
not many symbol locations come from main files (.cpp, etc) and when headers are
modified, the main files are not reindexed right now. So the draft store will
give better location for
malaperle updated this revision to Diff 140272.
malaperle added a comment.
Use the DraftStore for offset -> line/col when there is a draft available.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
malaperle added inline comments.
Comment at: clangd/SourceCode.cpp:110
+
+llvm::Optional offsetRangeToLocation(SourceManager ,
+ StringRef File,
MaskRay wrote:
> May I ask a question about the conversion between
malaperle added a comment.
The index changes are moved here: https://reviews.llvm.org/D44954 I haven't
changed the patch yet though, I just removed it from this one.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
___
MaskRay added inline comments.
Comment at: clangd/tool/ClangdMain.cpp:105
+static llvm::cl::opt LimitWorkspaceSymbolResult(
+"workspace-symbol-limit",
malaperle wrote:
> sammccall wrote:
> > the -completion-limit was mostly to control rollout, I'm not sure
MaskRay added a comment.
> Have you considered also allowing '.' and ' ' (space) as separators in the
> request? Having additional separators doesn't really hurt complexity of the
> implementation, but allows to switch between tools for different languages
> easier.
I would suggest allowing
malaperle updated this revision to Diff 139968.
malaperle marked 4 inline comments as done.
malaperle added a comment.
Split the patch so that this part only has the workspace/symbol part
and the index model will be updated separately.
Repository:
rCTE Clang Tools Extra
malaperle marked 9 inline comments as done.
malaperle added a comment.
In https://reviews.llvm.org/D44882#1048355, @sammccall wrote:
> Can we split this patch up (it's pretty large anyway) and land the
> `workspaceSymbols` implementation first, without changing the indexer
> behavior?
>
> I
ilya-biryukov added a comment.
> I'm highly in favor of enabling fuzzy matching for `workspaceSymbols`.
At lest for the name themselves. Non-fuzzy matching of qualifiers does not look
that important.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
ilya-biryukov added a comment.
>>> - Current `fuzzyFind` implementation can only match qualifiers strictly
>>> (i.e. st::vector won't match std::vector). Should we look into allowing
>>> fuzzy matches for this feature? (not in this patch, rather in the long
>>> term).
>>
>> I'm not sure, I'm
sammccall added a comment.
Very nice! I'd like to reduce the scope of the initial patch, which seems to be
possible, so we can review the details but not get bogged down too much.
In https://reviews.llvm.org/D44882#1048179, @malaperle wrote:
> In https://reviews.llvm.org/D44882#1048043,
malaperle added inline comments.
Comment at: clangd/WorkspaceSymbols.cpp:165
+[](const SymbolInformation , const SymbolInformation ) {
+ return A.name < B.name;
+});
ilya-biryukov wrote:
> We have `FuzzyMatcher`, it can
malaperle added a comment.
In https://reviews.llvm.org/D44882#1048043, @ilya-biryukov wrote:
> - Unconditionally storing much more symbols in the index might have subtle
> performance implications, especially for our internal use (the codebase is
> huge). I bet that internally we wouldn't
ilya-biryukov added reviewers: sammccall, ilya-biryukov.
ilya-biryukov added a subscriber: sammccall.
ilya-biryukov added a comment.
Adding @sammccall, he will definitely want to take a look at index-related
changes.
On a high level, the approach seems just right.
A few questions immedieately
malaperle created this revision.
Herald added subscribers: cfe-commits, MaskRay, ioeric, jkorous-apple, mgrang,
ilya-biryukov, mgorny, klimek.
This is a basic implementation of the "workspace/symbol" request which is
used to find symbols by a string query. Since this is similar to code
52 matches
Mail list logo
