ilya-biryukov added a comment.
There are still a few nits I haven't addressed, but the other big change is now
there: removing ASTBuilder, replacing it with free-standing functions. I'd say
I liked the previous code better, since the use sites were a bit smaller and
the functions have ridiculou
ilya-biryukov updated this revision to Diff 148835.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
- Address review comments.
- Added one more test.
Repository:
rC Clang
https://reviews.llvm.org/D44480
Files:
lib/Sema/SemaDecl.cpp
test/CodeCompletion/crash
ilya-biryukov added a comment.
Sorry for delays with this patch, I somehow missed the email notification about
it.
> Expounding on my concerns a bit -- I'm worried about all the other places
> calling isUndeducedType() and whether they're also at risk and thus a better
> fix is to change isUnd
ilya-biryukov updated this revision to Diff 149073.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
- Added a unit test
- Address review comments
- Add ASTRetentionPolicy param to ClangdServer
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47063
F
ilya-biryukov added a comment.
I think your change makes sense, but maybe asking for a better description.
It appears you:
- Enabled crash recovery for some libclang operations on a calling thread even
when `LIBCLANG_NOTHREAD` is specified. Previously it would only run under
crash recovery if
ilya-biryukov added a comment.
Sorry for delays
@malaperle, thanks for submitting the patch.
Repository:
rL LLVM
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinf
ilya-biryukov added a comment.
Sorry for late response.
Comment at: clangd/ClangdUnit.cpp:610
+ParameterInformation Info;
+OptionalParameterLabel = getOptionalString(*Chunk.Optional);
+Result.label += OptionalParameterLabel;
Are we first
ilya-biryukov added inline comments.
Comment at: clangd/GlobalCompilationDatabase.cpp:82
// FIXME(ibiryukov): Invalidate cached compilation databases on changes
+return Result;
Why remove this `FIXME`?
Comment at: clangd/tool/ClangdM
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM.
PS I actually remember arguing to keep those classes separate in the original
review :-)
https://reviews.llvm.org/D38414
__
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.h:41
private:
- class LSPProtocolCallbacks;
- class LSPDiagnosticsConsumer : public DiagnosticsConsumer {
- public:
-LSPDiagnosticsConsumer(ClangdLSPServer &Server);
-
-virtual void
-onDiagnos
ilya-biryukov added a comment.
Friendly ping.
https://reviews.llvm.org/D37970
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
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,
+
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.cpp:742
+ Consumer->includeBriefComments();
+ FrontendOpts.CodeCompleteOpts.IncludeBriefComments =
+ Consumer->includeBriefComments();
Duplicated line sneaked into commit. It looks like
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Some changes seem to be lost while merging with head.
Comment at: clangd/GlobalCompilationDatabase.cpp:75
+ auto CachedIt = CompilationDatabases.find
ilya-biryukov requested changes to this revision.
ilya-biryukov added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/GlobalCompilationDatabase.cpp:90
+
+ Logger.log("Failed to find compilation database for " + Twine(File) + "\n");
+ return nu
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
Thanks. LGTM modulo minor NIT (see other inline comment).
Do you need help to land this?
Comment at: clangd/GlobalCompilationDatabase.cpp:102
+if (ReturnValue == nullptr)
+ Logger.log("Failed to find
ilya-biryukov added a comment.
Thanks for fixing the last comment.
Do you want me to land this for you?
Comment at: clangd/GlobalCompilationDatabase.cpp:103
+ Logger.log("Failed to find compilation database for " + Twine(File) +
+ "in overriden directory "
ilya-biryukov added a comment.
Did a minor rename and added a few `std::move`s before submitting. Was not
worth another round of code review.
Repository:
rL LLVM
https://reviews.llvm.org/D37150
___
cfe-commits mailing list
cfe-commits@lists.llvm
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. Do you need my help in landing this?
> 1. If it finds a callable, provide the name of the callable, followed by an
> opening parenthesis, followed by `$0`, followed by a cl
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.h:47
// Implement ProtocolCallbacks.
- void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutput &Out) override;
- void onShutdown(JSONOutput &Out) override;
- void onDocument
ilya-biryukov created this revision.
Herald added a subscriber: eraman.
Adjusted PrintingPolicy inside code completion to avoid printing some
redundant name qualifiers.
Before this change, typedefs that were written unqualified in source
code were printed with qualifiers in completion. For exampl
ilya-biryukov added inline comments.
Comment at: test/CodeCompletion/enum-switch-case-qualified.cpp:25
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:23:8 %s -o - |
FileCheck -check-prefix=CHECK-CC1 %s
-// CHECK-CC1: Blue : [#M::N::C::Color#]N::C::Blue
-//
ilya-biryukov added a comment.
@francisco.lopes, thanks for sharing your experience, I definitely like the
final UX you achieved, we should aim for something similar in clangd.
It also looks like @rwols's idea of how things should look like is very close
to your implementation.
I do think that
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.h:47
// Implement ProtocolCallbacks.
- void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutput &Out) override;
- void onShutdown(JSONOutput &Out) override;
- void onDocument
ilya-biryukov created this revision.
ClangdServer now provides async code completion API.
It is still used synchronously by ClangdLSPServer, more work is needed
to allow processing other requests in parallel while completion (or
any other request) is running.
https://reviews.llvm.org/D38583
Fil
ilya-biryukov updated this revision to Diff 117811.
ilya-biryukov added a comment.
- Moved 'const' to the left.
https://reviews.llvm.org/D38583
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/Clang
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:222
+
+ std::shared_ptr Preamble =
+ Resources->getPossiblyStalePreamble();
klimek wrote:
> I think we use "const type" everywhere.
Sorry, my previously preferred style keeps sneaking
ilya-biryukov updated this revision to Diff 117820.
ilya-biryukov added a comment.
- Added a comment about Preamble assignment.
https://reviews.llvm.org/D38583
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
unittests/clangd/ClangdTests.cpp
Index: unitte
ilya-biryukov updated this revision to Diff 117822.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
- Fixed grammar in a comment.
https://reviews.llvm.org/D38583
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
unittests/clan
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.h:236-237
+ ///
+ /// Request is processed asynchronously, you can use returned future to wait
+ /// for results of async request.
+ ///
klimek wrote:
> Extra space before asynchronously.
>
ilya-biryukov updated this revision to Diff 117843.
ilya-biryukov added a comment.
- Added another missing 'the'.
https://reviews.llvm.org/D38583
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/Cla
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. Only a few code style-related comments.
Comment at: include/clang/Lex/Preprocessor.h:102
+bool ReachedEOFWhileSkipping;
+SourceLocation HashToken;
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Also, one more important thing. I'll unaccept revision for now, before getting
feedback on this one.
Comment at: include/clang/Lex/Preprocessor.h:333
ilya-biryukov created this revision.
It was previsouly set only in ASTUnit, but it should be set for all client of
PrecompiledPreamble.
https://reviews.llvm.org/D38617
Files:
lib/Frontend/ASTUnit.cpp
lib/Frontend/PrecompiledPreamble.cpp
Index: lib/Frontend/PrecompiledPreamble.cpp
ilya-biryukov added a comment.
This commit will unbreak clangd from all the same issues that were fixed by
conditional stack commits.
https://reviews.llvm.org/D38617
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-
ilya-biryukov added a comment.
@francisco.lopes, I've surely noticed there are a bunch of places where
`signatureHelp` does not show up while I was playing around with this change.
It would be great to have it available in more cases.
Repository:
rL LLVM
https://reviews.llvm.org/D38048
__
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM.
Note there's a new LSP method handler added upstream
(`textDocument/signatureHelp`), we should add it to this change before
submitting.
Comment at: clan
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
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM.
https://reviews.llvm.org/D38578
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman
ilya-biryukov resigned from this revision.
ilya-biryukov added a comment.
I'm not actually familiar with `CXCursor`, so can't really help with reviewing
this.
https://reviews.llvm.org/D38615
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
h
ilya-biryukov created this revision.
They are now used in ClangdScheduler instead of deferred std::async
computations.
The results of `std::async` are much less effective and do not provide
a good abstraction for similar purposes, i.e. for storing additional callbacks
to clangd async tasks. The ac
ilya-biryukov updated this revision to Diff 118008.
ilya-biryukov added a comment.
- Fix to ForwardBinder.
- Add UniqueFunction(nullptr) constructor.
- Added missing STL headers to Function.h
https://reviews.llvm.org/D38627
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Funct
ilya-biryukov created this revision.
https://reviews.llvm.org/D38629
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -252,6 +252,14 @@
ilya-biryukov added a comment.
I hope the whole design doesn't seem overly complicated. Very much open to
suggestions on how to simplify it :-)
https://reviews.llvm.org/D38629
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llv
ilya-biryukov added inline comments.
Comment at: clangd/Function.h:36
+ template
+ UniqueFunction(Callable Func)
+ : CallablePtr(llvm::make_unique<
sammccall wrote:
> Do you want this constructor to be explicit?
>
> If not, I think you should be able to
ilya-biryukov updated this revision to Diff 118204.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.
Addressed review comments.
- Added a file comment.
- Simplified callsites of UniqueFunction in ClangdServer.h
- Properly forward UniqueFunction's constructor argument
ilya-biryukov added reviewers: bkramer, klimek.
ilya-biryukov added a comment.
Adding more reviewers, in case someone will have time to look at that :-)
https://reviews.llvm.org/D38617
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://l
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
Looking forward to getting this change! I miss this as well.
Please take a look at my comments, though. I think we might want to use a
different API to implement this.
Comment at: clangd/ClangdSer
ilya-biryukov updated this revision to Diff 118205.
ilya-biryukov added a comment.
- Included more cases into a qualifiers-as-written.cpp test.
https://reviews.llvm.org/D38538
Files:
include/clang/AST/QualTypeNames.h
include/clang/Tooling/Core/QualTypeNames.h
lib/AST/CMakeLists.txt
lib/
ilya-biryukov added inline comments.
Comment at: test/CodeCompletion/call.cpp:22
// CHECK-CC1: COMPLETION: Pattern : dynamic_cast<<#type#>>(<#expression#>)
- // CHECK-CC1: f(N::Y y, <#int ZZ#>)
+ // CHECK-CC1: f(Y y, <#int ZZ#>)
// CHECK-CC1-NEXT: f(int i, <#int j#>, int
ilya-biryukov updated this revision to Diff 118184.
ilya-biryukov added a comment.
- Use proper types (Args&&) when forwarding arguments.
https://reviews.llvm.org/D38627
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Function.h
Index: clangd/Function.h
==
ilya-biryukov updated this revision to Diff 118185.
ilya-biryukov added a comment.
Herald added a subscriber: mgorny.
- Restore qualifiers for types of EnumConstantDecl.
https://reviews.llvm.org/D38538
Files:
include/clang/AST/QualTypeNames.h
include/clang/Tooling/Core/QualTypeNames.h
lib
ilya-biryukov added a comment.
In https://reviews.llvm.org/D38617#892092, @bkramer wrote:
> A testcase would be nice, but this can go in to unblock things.
Thanks for reviewing this!
Added a test case to clangd in https://reviews.llvm.org/rL315213.
Repository:
rL LLVM
https://reviews.llvm.
ilya-biryukov added inline comments.
Comment at: test/CodeCompletion/enum-switch-case-qualified.cpp:25
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:23:8 %s -o - |
FileCheck -check-prefix=CHECK-CC1 %s
-// CHECK-CC1: Blue : [#M::N::C::Color#]N::C::Blue
-//
ilya-biryukov updated this revision to Diff 118225.
ilya-biryukov added a comment.
- Added a deprecation notice to function description.
https://reviews.llvm.org/D38629
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
Index: clangd/ClangdServer.h
===
ilya-biryukov added a comment.
We had a discussion with @sammccall offline, probably
> - what's the goal? Make the code read more naturally, change the async model,
> or something else?
Callback API is more flexible (if `std::future` that we use had `then`, they'd
be equivalent).
We have inte
ilya-biryukov added a comment.
In https://reviews.llvm.org/D38627#892773, @chapuni wrote:
> Excuse me, Ubuntu 14.04's g++-4.8.4 doesn't compile it. :(
> Any idea?
Sorry about that. Fixed in https://reviews.llvm.org/rL315284.
clang and gcc (at least version 4.8) behave differently here, so I di
ilya-biryukov created this revision.
https://reviews.llvm.org/D38720
Files:
clangd/ClangdUnit.cpp
test/clangd/completion-items-kinds.test
Index: test/clangd/completion-items-kinds.test
===
--- /dev/null
+++ test/clangd/completi
ilya-biryukov created this revision.
https://reviews.llvm.org/D38731
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
ilya-biryukov added inline comments.
Comment at: test/CodeCompletion/qualifiers-as-written.cpp:29
+ // CHECK-2: COMPLETION: func : [#int#]func(<#foo a#>, <#bar b#>, <#ns::bar
c#>, <#ns::baz d#>
+ // CHECK-2: COMPLETION: func : [#int#]func(<#foo::type a#>, <#bar b#>, <#baz
c#>
ilya-biryukov updated this revision to Diff 119813.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
- Inlined makeFutureAPIFromCallback.
https://reviews.llvm.org/D38629
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
Index: clangd/ClangdServer.h
ilya-biryukov added inline comments.
Comment at: clangd/Trace.cpp:87
+
+static Tracer* T = nullptr;
+} // namespace
Maybe use `unique_ptr` (or even `llvm::Optional`)? Otherwise we
leak memory and don't flush the stream if users of tracing API forget to call
`st
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:51
+template
+std::future makeFutureAPIFromCallback(
sammccall wrote:
> I'm not sure we need a template here rather than just writing the code
> directly.
Removed the template, inlined i
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:500
+if (!RefactoringClient)
+ RefactoringClient =
llvm::make_unique();
+Results = clangd::findAvailableRefactoringCommands(*RefactoringClient,
*AST,
Maybe initialize `Refacto
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:59
void ClangdLSPServer::onShutdown(Ctx C, ShutdownParams &Params) {
- IsDone = true;
+ // Before we reply, we could serialize the preambles to disk. For now, let's
+ // just say we're ready to exit.
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.cpp:103
+ void AfterExecute(CompilerInstance &CI) override {
+const SourceManager &SM = CI.getSourceManager();
Nebiroth wrote:
> ilya-biryukov wrote:
> > There's a much better public API to
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
Oh, and other than those NITs, LGTM.
@rwols, you have commit access now, right?)
https://reviews.llvm.org/D38939
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lis
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?
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`
ilya-biryukov added a comment.
Friendly ping.
Would it be okay to make the new behaviour optional? (Leaving old behaviour to
be the default).
https://reviews.llvm.org/D38538
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm
ilya-biryukov added a comment.
In https://reviews.llvm.org/D38731#898539, @krasimir wrote:
> Great! I like the unit testing approach a lot!
Thanks!
Repository:
rL LLVM
https://reviews.llvm.org/D38731
___
cfe-commits mailing list
cfe-commits@li
ilya-biryukov added a comment.
In https://reviews.llvm.org/D37554#903401, @nik wrote:
> Ilya, I hope it's OK if I take your description :)
Sure, no problem.
https://reviews.llvm.org/D37554
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
h
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 que
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 https://reviews.llvm.org/D
ilya-biryukov added inline comments.
Comment at: clangd/Trace.cpp:87
+
+static Tracer* T = nullptr;
+} // namespace
sammccall wrote:
> ilya-biryukov wrote:
> > Maybe use `unique_ptr` (or even `llvm::Optional`)?
> > Otherwise we leak memory and don't flush the st
ilya-biryukov created this revision.
Herald added a subscriber: mgorny.
For code reuse in SemaCodeComplete.
Note that the tests for QualTypeNames are still in Tooling as they use
Tooling's common testing code.
https://reviews.llvm.org/D39224
Files:
include/clang/AST/QualTypeNames.h
include/
ilya-biryukov updated this revision to Diff 120016.
ilya-biryukov added a comment.
- Extracted the move of QualTypeNames to AST into a separate revision.
https://reviews.llvm.org/D38538
Files:
lib/Sema/SemaCodeComplete.cpp
test/CodeCompletion/call.cpp
test/CodeCompletion/qualifiers-as-wri
ilya-biryukov added a comment.
The actual usage of `QualTypeNames` is in https://reviews.llvm.org/D38538.
https://reviews.llvm.org/D39224
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm
ilya-biryukov added a comment.
In https://reviews.llvm.org/D39224#905431, @rnk wrote:
> Can you remind me why `NamedDecl::printQualifiedName` is not appropriate for
> your needs?
The use-case in code completion (see https://reviews.llvm.org/D38538, the
interesting bit is in `SemaCodeComplete
ilya-biryukov added a comment.
In https://reviews.llvm.org/D38939#904318, @rwols wrote:
> I'll have to think about if I want that responsibility!
While you're at it, I'll submit this patch for you. But I'd definitely
encourage you to get commit access. You've been contributing a bunch of usefu
ilya-biryukov added a subscriber: malaperle.
ilya-biryukov added a comment.
There's another patch (https://reviews.llvm.org/D39276) that tries to add
`workspace/executeCommand` for a slightly different use-case.
Could we take the code for parsing/handling `workspace/executeCommand` from
this pat
ilya-biryukov created this revision.
ilya-biryukov added a reviewer: ioeric.
Herald added a subscriber: kadircet.
Used in clangd's symbol builder to optimize for the common
shared-memory executor case.
Repository:
rC Clang
https://reviews.llvm.org/D51164
Files:
include/clang/Tooling/AllTUs
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51163
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ilya-biryukov updated this revision to Diff 162194.
ilya-biryukov added a comment.
- Don't merge on-the-fly for multi-process executors
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51155
Files:
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
Index: clangd/global-s
ilya-biryukov added inline comments.
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:61
+/// rely on MR use-case to work properly.
+llvm::cl::init(false));
+
ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > ilya-biryukov w
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM from my side. We need this to unbreak performance in code completion
Repository:
rC Clang
https://reviews.llvm.org/D51159
_
ilya-biryukov added a comment.
Mostly NITs, but please take a look at the potential data race
Comment at: clangd/Cancellation.cpp:23
+const Task &getCurrentTask() {
+ return *Context::current().getExisting(TaskKey);
+}
Maybe add 'assert' that TaskKey is presen
ilya-biryukov updated this revision to Diff 162338.
ilya-biryukov added a comment.
- Make the option hidden
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51155
Files:
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
Index: clangd/global-symbol-builder/GlobalSymbolBu
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:159
+ }
+ if (SpecFuzzyReq) {
+if (auto Filter = speculateCompletionFilter(Content, Pos)) {
ioeric wrote:
> ilya-biryukov wrote:
> > NIT: maybe invert condition to reduce nesting?
> It
ilya-biryukov added a comment.
@Dmitry.Kozhevnikov, do you need us to land the patch?
Any last changes you'd like to make?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50993
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ilya-biryukov added inline comments.
Comment at: clangd/CodeComplete.h:187
+/// A speculative and asynchronous fuzzy find index request (based on cached
+/// request) that can be before parsing sema. This would reduce completion
+/// latency if the speculation succeeds.
-
ilya-biryukov added inline comments.
Comment at: clangd/CodeComplete.h:187
+/// A speculative and asynchronous fuzzy find index request (based on cached
+/// request) that can be before parsing sema. This would reduce completion
+/// latency if the speculation succeeds.
-
ilya-biryukov added a comment.
Parts of the patch contain were landed as https://reviews.llvm.org/D50847 and
https://reviews.llvm.org/D50889, happy to discuss the design decisions
("merged" dynamic index, two callbacks), but that's probably out of scope of
this patch.
Repository:
rCTE Clang
ilya-biryukov added a comment.
In https://reviews.llvm.org/D50993#1212162, @Dmitry.Kozhevnikov wrote:
> In https://reviews.llvm.org/D50993#1212130, @ilya-biryukov wrote:
>
> > @Dmitry.Kozhevnikov, do you need us to land the patch?
> > Any last changes you'd like to make?
>
>
> It’s blocked by th
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LG, thanks. And two small NITs.
Comment at: clangd/CodeComplete.h:184
+llvm::Expected
+speculateCompletionFilter(llvm::StringRef Content, Position Pos);
+
-
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LG with a few small comments
Comment at: clangd/Protocol.cpp:635
+ if(const auto AsNumber = Params->getAsInteger())
+return utostr(AsNumber.getValue());
+
ilya-biryukov added a subscriber: sammccall.
ilya-biryukov added inline comments.
Comment at: clangd/tool/ClangdMain.cpp:197
+static llvm::cl::opt IncludeFixIts(
+"include-fixits",
I wonder if we should make the `IncludeFixIts` option hidden?
It currently o
ilya-biryukov added inline comments.
Comment at: clangd/tool/ClangdMain.cpp:197
+static llvm::cl::opt IncludeFixIts(
+"include-fixits",
sammccall wrote:
> ilya-biryukov wrote:
> > I wonder if we should make the `IncludeFixIts` option hidden?
> > It currentl
ilya-biryukov added a comment.
Thanks for the cleanups, mostly NITs from my side.
Comment at: clangd/ClangdServer.cpp:81
- SymbolIndex &index() const { return *MergedIndex; }
+ SymbolIndex &index() { return *MergedIndex; }
Maybe return `const SymbolIndex&
ilya-biryukov added a comment.
Sorry for chiming in, wanted to add my 2 cents to the conversation.
In https://reviews.llvm.org/D50462#1206203, @vsapsai wrote:
> What about having a mode that treats missing header as non-fatal error?
> Because I believe there are cases where there is no sense to
201 - 300 of 3283 matches
Mail list logo
