This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Cancel certain operations if the file changes before we start.
ClosedPublic

Authored by sammccall on Mar 4 2020, 5:10 AM.

Details

Summary

Otherwise they can force us to build lots of snapshots that we don't need.
Particularly, try to do this for operations that are frequently
generated by editors without explicit user interaction, and where
editing the file makes the result less useful. (Code action
enumeration is a good example).

https://github.com/clangd/clangd/issues/298

This doesn't return the "right" LSP error code (ContentModified) to the client,
we need to teach the cancellation API to distinguish between different causes.

Diff Detail

Event Timeline

sammccall created this revision.Mar 4 2020, 5:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2020, 5:10 AM
kadircet accepted this revision.Mar 4 2020, 7:44 AM

thanks, LGTM

clang-tools-extra/clangd/Cancellation.cpp
35–39

comment seems to be out-of-date.

Either not cancelled or not in a cancellable task ?

clang-tools-extra/clangd/JSONTransport.cpp
23

s/approprate/appropriate/

clang-tools-extra/clangd/TUScheduler.cpp
673

braces

682

this one is already being put into the context before pushing into requests.

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
364

nit: bool(AST) ? same in other places.

This revision is now accepted and ready to land.Mar 4 2020, 7:44 AM
kadircet added inline comments.Mar 4 2020, 8:40 AM
clang-tools-extra/clangd/ClangdServer.cpp
58

nit: i would actually keep InvalidateOnUpdate on the callsites, but up to you.

also constexpr instead of static ?

sammccall updated this revision to Diff 248336.Mar 4 2020, 3:09 PM
sammccall marked 7 inline comments as done.

address comments

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Mar 4 2020, 3:36 PM

This broke building on my bots:

http://45.33.8.238/linux/11862/step_4.txt

ld.lld: error: undefined symbol: clang::clangd::TUScheduler::InvalidatedError::ID

Looks like that is indeed declared in this change, but it doesn't have a definition.

This broke building on my bots:

http://45.33.8.238/linux/11862/step_4.txt

ld.lld: error: undefined symbol: clang::clangd::TUScheduler::InvalidatedError::ID

Looks like that is indeed declared in this change, but it doesn't have a definition.

Sorry about that, fixing. (I didn't see this on other linkers, maybe because the class itself wasn't used in this patch)

clang-tools-extra/clangd/Cancellation.cpp
35–39

Yeah, just removed the comment, this is now pretty obvious.