sammccall added subscribers: ilya-biryukov, jkorous.
sammccall added a comment.
Couple of thoughts. (Technically I'm out on leave so will let Jan/Ilya
review implementation and happy with whatever you decide)
Enabling
- negotiating LSP extensions is probably better done in the "capabilities"
me
sammccall created this revision.
sammccall added reviewers: ioeric, klimek.
Herald added a subscriber: cfe-commits.
Rather than hold all the JSON source in memory, as well as the YAML structures
needed to index into it, we parse eagerly into CompileCommand structures, which
simplifies the implemen
sammccall updated this revision to Diff 159690.
sammccall added a comment.
(just updating description)
Repository:
rC Clang
https://reviews.llvm.org/D50439
Files:
include/clang/Tooling/JSONCompilationDatabase.h
lib/Tooling/JSONCompilationDatabase.cpp
test/Index/skip-parsed-bodies/compi
sammccall planned changes to this revision.
sammccall added a comment.
Nevermind, this is like 5x slower to load the chromium CDB. probably because of
all the little non-arena allocations.
I'll profile and fix it when I get bored again.
Repository:
rC Clang
https://reviews.llvm.org/D50439
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:105
+size_t MemIndex::estimateMemoryUsage() {
+ size_t Bytes = Index.size() * sizeof(std::pair);
+ return Bytes / (1000 * 1000);
access to Index needs to be guarded by mute
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
This part looks good (I think everything controversial got moved into the other
patch). Thanks!
Comment at: clangd/Threading.cpp:75
+std::string ThreadName;
+d
sammccall added a comment.
first few comments, sorry I'll need to come back to this.
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:90
virtual DocID peek() const = 0;
/// Retrieves boosting score. Query tree root should pass Root->peek() to
this
/// functio
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/CodeComplete.cpp:810
+return false;
+ auto Scope = CC.getCXXScopeSpecifier();
+ if (!Scope)
we could use a high level comm
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Looks good, nits as always and I think you want a new test case.
Comment at: clangd/CodeComplete.cpp:817
Result.IncludeGlobals = true;
- Result.IncludeBriefComments
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
This is pretty small and seems unlikely to be controversial. I think it's OK to
commit at this point, it's been a few weeks.
Comment at: include/clang/Sema/CodeComplet
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/ClangdUnit.h:53
std::vector Diags;
- InclusionLocations IncLocations;
+ std::vector Inclusions;
};
this might be a good pl
sammccall updated this revision to Diff 146584.
sammccall added a comment.
fix diffbase?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46524
Files:
clangd/CMakeLists.txt
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/CMakeLists.txt
uni
sammccall updated this revision to Diff 146591.
sammccall marked an inline comment as done.
sammccall added a comment.
Doxygen
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46524
Files:
clangd/CMakeLists.txt
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
sammccall added inline comments.
Comment at: clangd/CodeComplete.cpp:247
+// Calculates include path and insertion edit for a new header.
+class IncludeInserter {
+public:
Do you think it would make sense to move this class to Headers.h? This would
reduce the no
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Nice, ship it!
Comment at: clangd/CodeComplete.cpp:317
+if (Includes && !D->IncludeHeader.empty()) {
+ // Fallback to canonical header if declaration l
sammccall added a comment.
@malaperle to expand on what Eric said, adding proto hacks with false positives
and no way to turn them off is indeed not the way to go here!
There's probably going to be other places we want to filter symbols too, and it
should probably be extensible/customizable in s
sammccall added a comment.
In https://reviews.llvm.org/D46751#1099633, @malaperle wrote:
> In https://reviews.llvm.org/D46751#1099235, @sammccall wrote:
>
> > @malaperle to expand on what Eric said, adding proto hacks with false
> > positives and no way to turn them off is indeed not the way to
This revision was automatically updated to reflect the committed changes.
Closed by commit rL332378: [clangd] Extract scoring/ranking logic, and shave
yaks. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D46524?
sammccall added inline comments.
Comment at: clangd/AST.h:32
+/// Returns true iff all redecls of \p D are in the main file.
+bool allDeclsInMainFile(const Decl *D);
Do you expect this to be reused?
If so, unit test? Otherwise this seems small enough to move wh
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
n
Comment at: clangd/index/CanonicalIncludes.cpp:50
+ if (I == Headers.end())
+return Headers[0]; // Fallback to the defining header.
+ llvm::StringRef Header = *
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks for chasing this, good detective work!
Changes look plausible and tests are nice, so LG assuming you know what you're
doing. If you're unsure, Manuel will give you better advice th
sammccall added a comment.
(Haven't reviewed the code yet, just design stuff)
I'm tempted to push back on the idea that we need to store any - "if it's fast
enough for code complete, it's fast enough for GTD". But that's probably
oversimplifying - we don't force reuse of a stable preamble for GT
sammccall added inline comments.
Comment at: clangd/index/CanonicalIncludes.cpp:54
+ StringRef Header = *I;
+ if (!isLiteralInclude(Header)) {
+// If Header is not expected be included (e.g. .cc file), we fall back to
headers are paths, not , right?
=
sammccall added a comment.
Having taken a closer look, I think the cache can be simplified/separated a bit
more cleanly by returning shared pointers and not allowing lookups, instead
restoring limited ownership in CppFile...
Happy to discuss more, esp if you might disagree :)
===
sammccall added inline comments.
Comment at: lib/Index/USRGeneration.cpp:691
+case BuiltinType::ULongAccum:
+ llvm_unreachable("No USR name mangling for fixed point types.");
case BuiltinType::Float16:
leonardchan wrote:
> rsmith wrote:
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/CodeCompletionStrings.h:46
const CodeCompleteConsumer::OverloadCandidate &Result,
- unsigned ArgInd
sammccall added a comment.
Thanks, this looks a lot better!
There do seem to be a lot of little classes that exist exactly one-per-TU
(ASTWorker, ASTBuilder, CachedAST, to a lesser extent ParsedAST) and I'm not
sure the balance of responsibilities is quite right. Some comments below.
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Nice, simple and will admit refinements later.
Just test nits and a trivial organizational thing.
Comment at: clangd/Quality.cpp:22
+namespace {
+bool hasDeclInMainF
sammccall added a comment.
Wow, nice!
Just unsure about the test helper. (We can rewrite it in another way if needed)
Comment at: clangd/ClangdUnit.h:88
- /// This function returns all top-level decls, including those that come
- /// from Preamble. Decls, coming from Preamb
sammccall added a comment.
In https://reviews.llvm.org/D47223#1110571, @malaperle wrote:
> In https://reviews.llvm.org/D47223#1109247, @ilya-biryukov wrote:
>
> > I'm not sure if we have tests for that, but I remember that we kept the
> > enumerators in the outer scope so that completion could f
sammccall added a comment.
The cache looks way simpler now, thank you!
As discussed offline, flattening ASTBuilder right into ASTWorker still seems
like a good idea to me, but happy with what you come up with there.
Comment at: clangd/TUScheduler.cpp:71
+
+ /// Update the fun
sammccall added a subscriber: hokein.
sammccall added inline comments.
Comment at: clangd/index/Index.h:158
unsigned References = 0;
-
+ /// Whether or not this is symbol is meant to be used for the global
+ /// completion.
ioeric wrote:
> s/this is symbol/t
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/CodeComplete.cpp:249
+ if (SemaResult->Kind == CodeCompletionResult::RK_Declaration) {
+if (const auto *D = SemaResult->getDeclarati
sammccall added a comment.
Impl looks good.
Is there a way we can reasonably test this? (specifically that we don't just
store all the ASTs forever)
Comment at: clangd/ClangdUnit.h:143
+std::shared_ptr
+buildPreamble(PathRef FileName, CompilerInvocation &CI,
+ std
sammccall added a comment.
Herald added a subscriber: jkorous.
Hey Simon,
Sorry this patch got stalled. I hope you're still interested!
I agree there's space for providing some high-signal information about error
conditions to power users/potential developers/administrators, even if it's not
"p
sammccall created this revision.
There doesn't seem to be any real separation between the current three objects.
Feel free to reject this if you find the current style valuable, though.
(Mostly I'm just looking around for cleanups to help me understand the code).
https://reviews.llvm.org/D38414
sammccall updated this revision to Diff 117256.
sammccall added a comment.
- Address review comments
https://reviews.llvm.org/D38414
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
Index: clangd/ClangdLSPServer.h
==
This revision was automatically updated to reflect the committed changes.
Closed by commit rL314587: [clangd] simplify ClangdLSPServer by
private-inheriting callback interfaces. NFC (authored by sammccall).
Repository:
rL LLVM
https://reviews.llvm.org/D38414
Files:
clang-tools-extra/trunk/c
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
This test looks like it was intended to catch some case, maybe we're now
mishandling some case like
if (foo)
{
}
else
{
// this can now be merged
}
But there's no testca
sammccall created this revision.
Make the ProtocolHandlers glue between JSONRPCDispatcher and
ClangdLSPServer generic.
Eliminate small differences between methods, de-emphasize the unimportant
distinction between notifications and methods.
ClangdLSPServer is no longer responsible for producing a
sammccall updated this revision to Diff 117374.
sammccall added a comment.
- clang-format
https://reviews.llvm.org/D38464
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/JSONRPCDispatcher.cpp
clangd/JSONRPCDispatcher.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd
sammccall updated this revision to Diff 117835.
sammccall marked 5 inline comments as done.
sammccall added a comment.
- ClangLSPServer.h should refer to ShutdownParams, not NoParams
- Address review comments
https://reviews.llvm.org/D38464
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSP
sammccall added a comment.
Thanks! All addressed (except removing ShutdownParams, which I'm not sure is
worthwhile).
Comment at: clangd/ClangdLSPServer.h:47
// Implement ProtocolCallbacks.
- void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutp
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/Function.h:9
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FUNCTION_
sammccall added a comment.
I'm missing some context for this one:
- what's the goal? Make the code read more naturally, change the async model,
or something else?
- what's the end state for codeComplete specifically? will we switch to the new
overload and delete the other, or is makeFutureAPIFr
sammccall updated this revision to Diff 118782.
sammccall added a comment.
Rebase
https://reviews.llvm.org/D38464
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/JSONRPCDispatcher.cpp
clangd/JSONRPCDispatcher.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/Protoco
This revision was automatically updated to reflect the committed changes.
Closed by commit rL315577: [clangd] less boilerplate in RPC dispatch (authored
by sammccall).
Repository:
rL LLVM
https://reviews.llvm.org/D38464
Files:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools
sammccall added inline comments.
Comment at: clangd/ClangdServer.cpp:51
+template
+std::future makeFutureAPIFromCallback(
I'm not sure we need a template here rather than just writing the code directly.
Comment at: clangd/ClangdServer.cpp:25
sammccall accepted this revision.
sammccall added a comment.
Going to land this on Ilya's behalf as he's out on vacation.
https://reviews.llvm.org/D38720
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/l
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: test/clangd/authority-less-uri.test:33
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+# CHECK: {"jsonrpc":"2.0","id":3,"result":null}
+Content-Length: 3
sammccall accepted this revision.
sammccall added a comment.
So I'm not totally convinced that treating an abrupt exit or EOF as an error is
worth the hassle (others might disagree), but it's certainly reasonable.
Could you add a test for the new behavior?
something like
RUN: not clangd -run-syn
sammccall added a comment.
Hi Alex! I'm working on clangd, but am pretty new to the project, so forgive
some naive questions.
I'm a bit unclear at a high level what the new abstractions in this patch add,
in terms of wiring refactorings up to clangd and other tools.
My understanding is: we have
sammccall created this revision.
Herald added a subscriber: mgorny.
This lets you visualize clangd's activity on different threads over time,
and understand critical paths of requests and object lifetimes.
The data produced can be visualized in Chrome (at chrome://tracing), or
in a standalone copy
sammccall added inline comments.
Comment at: clangd/JSONRPCDispatcher.cpp:230
- // Finally, execute the action for this JSON message.
- if (!Dispatcher.call(JSONRef, Out))
-Out.log("JSON dispatch failed!\n");
+ {
+// Finally, execute the action fo
sammccall updated this revision to Diff 119562.
sammccall marked 3 inline comments as done.
sammccall added a comment.
Address review comments.
https://reviews.llvm.org/D39086
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/JSONRPCDispatcher.cpp
clang
sammccall marked an inline comment as done.
sammccall added inline comments.
Comment at: clangd/Trace.cpp:87
+
+static Tracer* T = nullptr;
+} // namespace
ilya-biryukov wrote:
> Maybe use `unique_ptr` (or even `llvm::Optional`)? Otherwise
> we leak memory and d
sammccall updated this revision to Diff 119864.
sammccall added a comment.
Address FIXME now that r316330 is in.
https://reviews.llvm.org/D39086
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/JSONRPCDispatcher.cpp
clangd/ProtocolHandlers.cpp
clangd
sammccall added a comment.
As noted on the other patch, I'm not sure EditorCommands is a useful
abstraction.
This shows up one of the problems: clangd is mostly decoupled from the actual
behavior/semantics of the commands by the EditorCommands abstraction. However
LSP doesn't say what the stru
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:123
+ size_t Bytes = Index.estimateMemoryUsage();
+ for (const auto &Scheme : URISchemes) {
+// std::
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:109
+ float consume() override {
+if (reachedEnd())
return DEFAULT_BOOST_SCORE;
this isn't possible, as the item being consumed is the value of peek().
You co
sammccall accepted this revision.
sammccall added a comment.
Nice fix!
Comment at: lib/Format/Format.cpp:1312
auto &Line = *AnnotatedLines[i];
if (Line.startsWith(tok::kw_namespace) ||
+ Line.startsWith(tok::kw_inline, tok::kw_namespace) ||
--
sammccall added inline comments.
Comment at: unittests/Format/FormatTest.cpp:7582
Style);
+ verifyFormat("export namespace Foo\n"
+ "{};",
you may want to add tests for other modules TS syntax (e.g. non-namespace
export decls).
It
sammccall added a comment.
Mostly just reviewed the `references()` implementation.
For the infrastructure I have some high level questions:
- why are we combining SymbolSlab and occurrences?
- what's the motivation for the separate preamble/main-file indexes
- why put more functionality into Sym
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:34
+consumeIDs(std::unique_ptr It,
+ size_t Limit = std::numeric_limits::max()) {
+ auto
sammccall added a comment.
In https://reviews.llvm.org/D50958#1212179, @ilya-biryukov wrote:
> Parts of this patch 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 pr
sammccall added inline comments.
Comment at: clangd/index/Index.h:310
+
+ llvm::ArrayRef findOccurrences(const SymbolID &ID) const {
+auto It = SymbolOccurrences.find(ID);
As discussed offline: the merge of occurrences into SymbolSlab seems
problematic to m
sammccall added a comment.
The description says "LSP clients", i.e. editors.
For editors (e.g. whether editor can handle snippets), LSP capability
negotiation rather than flags is the right thing. I suspect this is true for
including completion fixes.
For users (e.g. I like behavior X), flags ar
sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ioeric, javed.absar.
- DynamicIndex doesn't implement ParsingCallbacks, to make its role clearer.
ParsingCallbacks is a separate object owned b
sammccall updated this revision to Diff 162401.
sammccall added a comment.
Add note about the overlap between preamble index across files.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51221
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
cla
sammccall added a comment.
This looks pretty good!
Comment at: clangd/index/Index.h:376
+
+ llvm::ArrayRef find(const SymbolID &ID) const {
+ auto It = Occurrences.find(ID);
assert frozen?
looking up in a non-frozen array is probably a mistake.
if we choo
sammccall added a comment.
Thanks for finding this problem, this fix *mostly* looks good (though I think
we can probably drop memoization).
I'm a bit uncomfortable about the places where we need the type, because this
is the only thing forcing us to parse before we've picked a command to
trans
sammccall added inline comments.
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:124
+// A CompileCommand that can be applied to another file. Any instance of this
+// object is invalid after std::move() from it.
struct TransferableCommand {
ilya-bi
sammccall added a comment.
In https://reviews.llvm.org/D51314#1215381, @sammccall wrote:
> I'm a bit uncomfortable about the places where we need the type, because this
> is the only thing forcing us to parse before we've picked a command to
> transfer, and the number of commands we need to par
sammccall added a comment.
In https://reviews.llvm.org/D51279#1214268, @ilya-biryukov wrote:
> Just noticed I'm not on the reviewers list, sorry for chiming in without
> invitation. Was really excited about the change :-)
fixed :-)
Comment at: clangd/index/FileIndex.cpp:58
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks!
Comment at: lib/Format/UnwrappedLineFormatter.cpp:486
return 0;
+if (Line.First->is(tok::kw_default)) {
+ const FormatToken *Tok = Line.First->g
sammccall accepted this revision.
sammccall added a comment.
In https://reviews.llvm.org/D51310#1214548, @kbobyrev wrote:
> It's probably better to roll out proximity path boosting & actual two-stage
> filtering before rolling this out.
Up to you (I don't understand the interaction), but this
sammccall accepted this revision.
sammccall added a comment.
Awesome :-)
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:220
-// CommandIndex does the real work: given a filename, it produces the best
-// matching TransferableCommand by matching filenames. Basic
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/tool/ClangdMain.cpp:263
+ // Use buffered stream to stderr. Unbuffered stream can cause significant
+ // (non-deterministic) latency for the lo
sammccall added a comment.
Nice!
We could reduce the scope of this patch somewhat by ignoring file proximity and
just switching to return the most popular header. This would be a solid
improvement over current behavior, and provide the infrastructure for the
file-proximity approach.
===
sammccall accepted this revision.
sammccall added inline comments.
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:229
+//
+// Apart from path proximity signals, also takes file extensions into account
+// when scoring the candidates.
(this comment i
sammccall added a comment.
Hi, sorry about overlooking cl mode flags when adding this, I was blissfully
unaware that `compile_commands.json` could use that syntax :-)
Out of curiosity, are you using this with clangd or some other tool? I'm sure
there are places where clangd injects unixy flags.
sammccall accepted this revision.
sammccall added inline comments.
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:127
// Language detected from -x or the filename.
- types::ID Type = types::TY_INVALID;
+ // When set, cannot be TY_INVALID.
+ llvm::Optional Type
sammccall added a comment.
Thanks for fixing this!
Mostly just picky style comments.
In particular, I know that some of the other maintainers here are just as
ignorant of the cl driver as I am, and I want to make sure that it's still
possible to follow the logic and debug unrelated problems with
sammccall marked 3 inline comments as done.
sammccall added inline comments.
Comment at: clangd/ClangdServer.cpp:122
+ // - symbols declared both in the main file and the preamble
+ // (Note that symbols *only* in the main file are not indexed).
FileIndex MainFileIdx;
-
sammccall added inline comments.
Comment at: clangd/TUScheduler.cpp:632
bool StorePreamblesInMemory,
- ParsingCallbacks &Callbacks,
+ std::unique_ptr Callbacks,
std::chrono::steady
sammccall updated this revision to Diff 163013.
sammccall marked an inline comment as done.
sammccall added a comment.
Address review comments, add missing dynamicIndex() implementation.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51221
Files:
clangd/ClangdServer.cpp
cla
sammccall updated this revision to Diff 163023.
sammccall added a comment.
(forgot some changes)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51221
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/TUScheduler.cpp
sammccall created this revision.
sammccall added a reviewer: kbobyrev.
Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous,
MaskRay, ioeric, ilya-biryukov.
This is now handled by a wrapper class SwapIndex, so MemIndex/DexIndex can be
immutable and focus on their job.
Old a
sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ioeric, javed.absar.
After code completion inserts a header, running signature help using the old
preamble will usually fail. So we add support
sammccall added a comment.
(This needs new TUScheduler tests for `Consistent` mode, but would like some
feedback on the implementation first)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51438
___
cfe-commits mailing list
cfe-com
sammccall updated this revision to Diff 163121.
sammccall added a comment.
Finish a sentence
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51438
Files:
clangd/ClangdServer.cpp
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/TUSchedulerTests.cpp
Index: uni
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/Protocol.h:832
+
+ /// Position of the opening paren of the argument list.
+ /// This is a clangd-specific extension, it is only available via
sammccall added a comment.
- It would be useful for the C++ API (CodeCompletion struct) to provide the
includes/edits in ranked order, if possible. Frontends could experiment with
showing all the options.
- Still not sure that adding more complicated behavior to Code Complete (vs
just improving
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:45
+ void build(std::shared_ptr> Symbols,
+ llvm::ArrayRef URISchemes);
ioeric wrote:
> URI schemes are property of `Symbols`. We shouldn't need to pass spec
sammccall updated this revision to Diff 163322.
sammccall marked 2 inline comments as done.
sammccall added a comment.
Herald added a subscriber: jfb.
Add test, address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51438
Files:
clangd/ClangdServer.cpp
clangd/TUSc
sammccall added a comment.
I don't have a strong opinion on async vs sync - startup time is important and
we shouldn't block simple AST-based functionality on the index, but this
introduces some slightly confusing UX for that speed.
However I think this should be based on https://reviews.llvm.o
sammccall added a comment.
(Sorry for catching this earlier, and that the patch isn't landed yet - feel
free to pick up the review, else @kbobyrev will take a first pass tomorrow I
think)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51475
_
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341076: [clangd] Run SignatureHelp using an up-to-date
preamble, waiting if needed. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.
sammccall added inline comments.
Comment at: clangd/TUScheduler.cpp:474
+llvm::unique_function)> Callback)
{
+ if (RunSync)
+return Callback(getPossiblyStalePreamble());
ilya-biryukov wrote:
> It seems we could remove the special-casing of `RunSync` and
sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ilya-biryukov.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51504
Files:
clangd/CodeComplete.cpp
clangd/global-symbol-builder/G
1 - 100 of 5927 matches
Mail list logo
