Summary:
Currently we delay AST rebuilds by 500ms after each edit, to wait for
further edits. This is a win if a rebuild takes 5s, and a loss if it
takes 50ms.
This patch sets debouncepolicy = clamp(min, ratio * rebuild_time, max).
However it sets min = max = 500ms so there's no policy change or actual
customizability - will do that in a separate patch.
See https://github.com/clangd/clangd/issues/275
Reviewers: hokein
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73873
Summary:
Default args might exist but be unparsed or uninstantiated.
getDefaultArg asserts on those. This patch makes sure we don't crash in such
scenarios.
Reviewers: sammccall, ilya-biryukov
Subscribers: MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73723
Summary:
Previously we mmapped on unix and not on windows: on windows mmap takes
an exclusive lock on the file and prevents the user saving changes!
The failure mode on linux is a bit more subtle: if the file is changed on disk
but the SourceManager sticks around, then subsequent operations on the
SourceManager will fail as invariants are violated (e.g. null-termination).
This commonly manifests as crashes after switching git branches with many files
open in clangd.
Nominally mmap is for performance here, and we should be willing to give some
up to stop crashing. Measurements on my system (linux+desktop+SSD) at least
show no measurable regression on an a fairly IO-heavy workload: drop disk caches,
open SemaOverload.cpp, wait for first diagnostics.
for i in `seq 100`; do
for variant in mmap volatile; do
echo 3 | sudo tee /proc/sys/vm/drop_caches
/usr/bin/time --append --quiet -o ~/timings -f "%C %E" \
bin/clangd.$variant -sync -background-index=0 < /tmp/mirror > /dev/null
done
done
bin/clangd.mmap -sync -background-index=0 0:07.60
bin/clangd.volatile -sync -background-index=0 0:07.89
bin/clangd.mmap -sync -background-index=0 0:07.44
bin/clangd.volatile -sync -background-index=0 0:07.89
bin/clangd.mmap -sync -background-index=0 0:07.42
bin/clangd.volatile -sync -background-index=0 0:07.50
bin/clangd.mmap -sync -background-index=0 0:07.90
bin/clangd.volatile -sync -background-index=0 0:07.53
bin/clangd.mmap -sync -background-index=0 0:07.64
bin/clangd.volatile -sync -background-index=0 0:07.55
bin/clangd.mmap -sync -background-index=0 0:07.75
bin/clangd.volatile -sync -background-index=0 0:07.47
bin/clangd.mmap -sync -background-index=0 0:07.90
bin/clangd.volatile -sync -background-index=0 0:07.50
bin/clangd.mmap -sync -background-index=0 0:07.81
bin/clangd.volatile -sync -background-index=0 0:07.95
bin/clangd.mmap -sync -background-index=0 0:07.55
bin/clangd.volatile -sync -background-index=0 0:07.65
bin/clangd.mmap -sync -background-index=0 0:08.15
bin/clangd.volatile -sync -background-index=0 0:07.54
bin/clangd.mmap -sync -background-index=0 0:07.78
bin/clangd.volatile -sync -background-index=0 0:07.61
bin/clangd.mmap -sync -background-index=0 0:07.78
bin/clangd.volatile -sync -background-index=0 0:07.55
bin/clangd.mmap -sync -background-index=0 0:07.41
bin/clangd.volatile -sync -background-index=0 0:07.40
bin/clangd.mmap -sync -background-index=0 0:07.54
bin/clangd.volatile -sync -background-index=0 0:07.42
bin/clangd.mmap -sync -background-index=0 0:07.45
bin/clangd.volatile -sync -background-index=0 0:07.49
bin/clangd.mmap -sync -background-index=0 0:07.95
bin/clangd.volatile -sync -background-index=0 0:07.66
bin/clangd.mmap -sync -background-index=0 0:08.04
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73617
Summary:
there is a slight behavior change in this patch:
- before: `in^t a;`, returns our internal error message (no symbol at given location)
- after: `in^t a, returns null, and client displays their message (e.g.
e.g. "the element can't be renamed" in vscode).
both are sensible according LSP, and we'd save one `rename` call in the later case.
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73610
Summary:
No need to pass fno-delayed-template-parsing as the opposite flag is
only passed to cc1 when abi is set to msvc. Sending as a follow-up to D73613 to
keep changes in the release branch minimal.
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73615
Summary:
This patch adds a simple mechanism to disallow global rename
on std symbols. We might extend it to other symbols, e.g. protobuf.
Reviewers: kadircet
Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73450
This has the same behavior as converting std::string_view to
std::string. This is an expensive conversion, so explicit conversions
are helpful for avoiding unneccessary string copies.
This is how it should've been and brings it more in line with
std::string_view. There should be no functional change here.
This is mostly mechanical from a custom clang-tidy check, with a lot of
manual fixups. It uncovers a lot of minor inefficiencies.
This doesn't actually modify StringRef yet, I'll do that in a follow-up.
Summary:
Currently clangd lit tests can't be run in isolation because we don't
set some of the config parameters. This enables running
./bin/llvm-lit ../clang-tools-extra/clangd/test/
or any other test in that subdirectory.
Reviewers: sammccall
Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73538
I just fixed a test involving a similar Notification class: 18e6a65bae
The pattern (notify() on one thread, wait() and then destroy the Notification
on the other) seems innocuous enough. I'm not sure we actually use it in clangd,
but better safe than sorry.
Summary:
This reflects its current function better and avoids confusion with clang::DiagnosticConsumer.
The old name/constructor is left around temporarily for compatibility.
(Metagame: merging with out-of-tree changes is harder than usual this month)
Reviewers: hokein
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73346
Summary:
Currently 🡺 is used in hover response to represent return types, but it
is not widely available. Changing this back to original to support more clients.
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73336
Summary:
It simply shows the completed/total items on the background queue, e.g.
indexing: 233/1000
The denominator is reset to zero every time the queue goes idle.
The protocol is fairly complicated here (requires creating a remote "progress"
resource before sending updates). We implement the full protocol, but I've added
an extension allowing it to be skipped to reduce the burden on clients - in
particular the lit test takes this shortcut.
The addition of background index progress to DiagnosticConsumer seems ridiculous
at first glance, but I believe that interface is trending in the direction of
"ClangdServer callbacks" anyway. It's due for a rename, but otherwise actually
fits.
Reviewers: kadircet, usaxena95
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, jfb, cfe-commits, llvm-commits
Tags: #clang, #llvm
Differential Revision: https://reviews.llvm.org/D73218
Summary:
The historic behavior of TestTU is to gather diagnostics and otherwise ignore
them. So if a test has a syntax error, and doesn't assert diagnostics, it
silently misbehaves.
This can be annoying when developing tests, as evidenced by various tests
gaining "assert no diagnostics" where that's not really the point of the test.
This patch aims to make that default behavior. For the first error
(not warning), TestTU will call ADD_FAILURE().
This can be suppressed with a comment containing "error-ok". For now that will
suppress any errors in the TU. We can make this stricter later -verify style.
(-verify itself is hard to reuse because of DiagnosticConsumer interfaces...)
A magic-comment was chosen over a TestTU option because of table-driven tests.
In addition to the behavior change, this patch:
- adds //error-ok where we're knowingly testing invalid code
(e.g. for diagnostics, crash-resilience, or token-level tests)
- fixes a bunch of errors in the checked-in tests, mostly trivial (missing ;)
- removes a bunch of now-redundant instances of "assert no diagnostics"
Reviewers: kadircet
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73199
Summary:
Some names, e.g. constructor/destructor/conversions, already contain
the type info, no need to duplicate them in the hoverinfo.
Fixes https://github.com/clangd/clangd/issues/252
Reviewers: sammccall, ilya-biryukov
Subscribers: MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73110
When triggering rename of the class name in the code with explicit destructor
calls, rename fails. Consider the following piece of code:
```
class Foo;
...
Foo f;
f.~/*...*/Foo();
```
`findExplicitReferences` will report two `ReferenceLoc` for destructor call:
one is comming from `MemberExpr` (i.e. destructor call itself) and would point
to the tilde:
```
f.~/*...*/Foo();
^
```
And the second one is pointing to the typename and is coming from `TypeLoc`.
```
f.~/*...*/Foo();
^
```
This causes rename to produce incorrect textual replacements. This patch
updates `MemberExpr` handler to detect destructor calls and prevents it
from reporting a duplicate reference.
Resolves: https://github.com/clangd/clangd/issues/236
Reviewers: kadircet, hokein
Differential Revision: https://reviews.llvm.org/D72638
Summary:
Printing policy was not propogated to functiondecls when creating a
completion string which resulted in canonical template parameters like
`foo<type-parameter-0-0>`. This patch propogates printing policy to those as
well.
Fixes https://github.com/clangd/clangd/issues/76
Reviewers: ilya-biryukov
Subscribers: jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D72715
The test is failing on our CI bots.
Seems like the order of results for one target is undefined.
(post-commit review)
Differential Revision: https://reviews.llvm.org/D72883
Sam McCall