[clangd] Allow embedders some control over when diagnostics are generated.

[sammccall]

Summary

Through the C++ API, we support for a given snapshot version:

• Yes: make sure we generate diagnostics for exactly this version
• Auto: generate eventually-consistent diagnostics for at least this version
• No: don't generate diagnostics for this version

Eventually auto should be debounced for better UX.

Through LSP, we force diagnostics for initial load (bypassing future debouncing)
and all updates follow the "auto" policy.

This is complicated to implement under the CancellationFlag design, so
rewrote that part to just inspect the queue instead.

It turns out we never pass None to the diagnostics callback, so remove Optional
from the signature. The questionable behavior of not invoking the callback at
all if CppFile::rebuild fails is not changed.

[ilya-biryukov]

clangd/ClangdServer.h

282 ↗

(On Diff #135104)

Maybe add a parameter name here?
It's mostly a personal preference, I tend to copy-paste parameter lists between declaration/definition site if I change them. Missing parameter names totally break this workflow for me :-)

clangd/TUScheduler.cpp

298 ↗

(On Diff #135104)

Instead of skipping requests here we could try removing them from back of the queue in startTask (or rewriting the last request instead of adding a new one).
It feels the code could be simpler, as we will only ever have to remove a single request from the queue. And it could also keep the queue smaller in case of many subsequent Auto requests.
WDYT?

339 ↗

(On Diff #135104)

Maybe skip updates directly in this function and make it return void?
Calling a function in a loop that loops through elements itself is a little confusing.

[sammccall]

add param names to decls

[sammccall]

Not convinced about the flow control stuff - I think you might be underestimating the complexity of the cancellation-based approach with the extra functionality.
But if you think it would still be more readable the other way, happy to get a third opinion.

clangd/ClangdServer.h

282 ↗

(On Diff #135104)

Done.
My preference is to optimise for reading rather than writing the code, and a name that carries no extra semantics is just noise. But I don't care that much.

clangd/TUScheduler.cpp

298 ↗

(On Diff #135104)

Having startTask look ahead to find things to cancel was the thing I found most confusing/limiting in the previous code, so I'd rather not go back there :-)
That said, I did try this first, trying to limit the scope of this patch, but it got hard.

The main problems are:

• you're not just looking ahead one task, or even to a fixed one. After [auto no], no cancels no, auto cancels both, read cancels neither. The states and the state machine are hard to reason about. (unless you just loop over the whole queue, which seems just as complex)
• the decision of "should task X run" is distributed over time via mutating state, rather than happening at one point via reads
  • when looking at startTask time, you have to reason about the (possibly) concurrently running task. In run(), no task is running and nothing can be enqueued, so there's no concurrency issues.

And it could also keep the queue smaller in case of many subsequent Auto requests.

This is true, but it doesn't seem like a practical concern.

339 ↗

(On Diff #135104)

Returning bool constrains the contract of this class to choosing to run one item or not, and the type system forces a specific decision.
Returning void leaves the option of purging one or no or multiple items. The only void function with a clear contract would be "drop all dead requests at the start", which is too complex to get right in one go. (happy to pull the loop out of run into such a function if you think it would help, though.)

[ilya-biryukov]

clangd/TUScheduler.cpp

298 ↗

(On Diff #135104)

Thanks for clarifying. The first bullet point shouldn't be a big a problem. Yes, the new task can remove multiple items from the back of the queue, but the implementation still looks more natural as it only needs to inspect the last item on the queue on each of the outer loop iterations. (While the current implementation has to do an inner loop through multiple items on the queue in addition to the outer loop).

The second point makes it hard, though. I would probably go with calling pop_front() when removing the request and signalling empty queue separately.

This is true, but it doesn't seem like a practical concern.

It isn't, but I still think it's a nice-to-have.

339 ↗

(On Diff #135104)

Another alternative is to return an iterator to the first non-dead request from this function use it to remove the dead requests in the clients of this function.
This would get rid of the outer loop. WDYT?

[ilya-biryukov]

LGTM (just one more possibly useful nit about const)

clangd/TUScheduler.cpp

105 ↗

(On Diff #135382)

NIT: this could be made const

298 ↗

(On Diff #135104)

As discussed offline, I totally missed the case when the new request comes in and has to cancel requests from the middle of the queue. So the implementation I proposed would probably still have two loops and not be less complex.

So LGTM here, my suggestions won't make things simpler.

clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp

 }
 
 void ClangdLSPServer::onExit(ExitParams &Params) { IsDone = true; }
 
 void ClangdLSPServer::onDocumentDidOpen(DidOpenTextDocumentParams &Params) {
   if (Params.metadata && !Params.metadata->extraFlags.empty())
     CDB.setExtraFlagsForFile(Params.textDocument.uri.file(),
                              std::move(Params.metadata->extraFlags));
-  Server.addDocument(Params.textDocument.uri.file(), Params.textDocument.text);
+  Server.addDocument(Params.textDocument.uri.file(), Params.textDocument.text,
+                     WantDiagnostics::Yes);
 }
 
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
   if (Params.contentChanges.size() != 1)
     return replyError(ErrorCode::InvalidParams,
                       "can only apply one change at a time");
   // We only support full syncing right now.
   Server.addDocument(Params.textDocument.uri.file(),
-                     Params.contentChanges[0].text);
+                     Params.contentChanges[0].text, WantDiagnostics::Auto);
 }
 
 void ClangdLSPServer::onFileEvent(DidChangeWatchedFilesParams &Params) {
   Server.onFileEvent(Params);
 }
 
 void ClangdLSPServer::onCommand(ExecuteCommandParams &Params) {
   auto ApplyEdit = [](WorkspaceEdit WE) {

clang-tools-extra/trunk/clangd/ClangdServer.h

public:
 
   /// Set the root path of the workspace.
   void setRootPath(PathRef RootPath);
 
   /// Add a \p File to the list of tracked C++ files or update the contents if
   /// \p File is already tracked. Also schedules parsing of the AST for it on a
   /// separate thread. When the parsing is complete, DiagConsumer passed in
   /// constructor will receive onDiagnosticsReady callback.
-  void addDocument(PathRef File, StringRef Contents);
+  void addDocument(PathRef File, StringRef Contents,
+                   WantDiagnostics WD = WantDiagnostics::Auto);
 
   /// Remove \p File from list of tracked files, schedule a request to free
   /// resources associated with it.
   void removeDocument(PathRef File);
 
   /// Force \p File to be reparsed using the latest contents.
   /// Will also check if CompileCommand, provided by GlobalCompilationDatabase
   /// for \p File has changed. If it has, will remove currently stored Preamble
private:
   /// FIXME: This stats several files to find a .clang-format file. I/O can be
   /// slow. Think of a way to cache this.
   llvm::Expected<tooling::Replacements>
   formatCode(llvm::StringRef Code, PathRef File,
              ArrayRef<tooling::Range> Ranges);
 
   void
   scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
+                          WantDiagnostics WD,
                           Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS);
 
   CompileArgsCache CompileArgs;
   DiagnosticsConsumer &DiagConsumer;
   FileSystemProvider &FSProvider;
   DraftStore DraftMgr;
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)

clang-tools-extra/trunk/clangd/ClangdServer.cpp

 
 void ClangdServer::setRootPath(PathRef RootPath) {
   std::string NewRootPath = llvm::sys::path::convert_to_slash(
       RootPath, llvm::sys::path::Style::posix);
   if (llvm::sys::fs::is_directory(NewRootPath))
     this->RootPath = NewRootPath;
 }
 
-void ClangdServer::addDocument(PathRef File, StringRef Contents) {
+void ClangdServer::addDocument(PathRef File, StringRef Contents,
+                               WantDiagnostics WantDiags) {
   DocVersion Version = DraftMgr.updateDraft(File, Contents);
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);
   scheduleReparseAndDiags(File, VersionedDraft{Version, Contents.str()},
-                          std::move(TaggedFS));
+                          WantDiags, std::move(TaggedFS));
 }
 
 void ClangdServer::removeDocument(PathRef File) {
   DraftMgr.removeDraft(File);
   CompileArgs.invalidate(File);
   WorkScheduler.remove(File);
 }
 
 void ClangdServer::forceReparse(PathRef File) {
   auto FileContents = DraftMgr.getDraft(File);
   assert(FileContents.Draft &&
          "forceReparse() was called for non-added document");
 
   // forceReparse promises to request new compilation flags from CDB, so we
   // remove any cahced flags.
   CompileArgs.invalidate(File);
 
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);
-  scheduleReparseAndDiags(File, std::move(FileContents), std::move(TaggedFS));
+  scheduleReparseAndDiags(File, std::move(FileContents), WantDiagnostics::Yes,
+                          std::move(TaggedFS));
 }
 
 void ClangdServer::codeComplete(
     PathRef File, Position Pos, const clangd::CodeCompleteOptions &Opts,
     UniqueFunction<void(Tagged<CompletionList>)> Callback,
     llvm::Optional<StringRef> OverridenContents,
     IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
   using CallbackType = UniqueFunction<void(Tagged<CompletionList>)>;
auto Action = [Pos, TaggedFS](decltype(Callback) Callback,
     Callback(make_tagged(std::move(Result), TaggedFS.Tag));
   };
 
   WorkScheduler.runWithAST("Hover", File,
                            BindWithForward(Action, std::move(Callback)));
 }
 
 void ClangdServer::scheduleReparseAndDiags(
-    PathRef File, VersionedDraft Contents,
+    PathRef File, VersionedDraft Contents, WantDiagnostics WantDiags,
     Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS) {
   tooling::CompileCommand Command = CompileArgs.getCompileCommand(File);
 
-  using OptDiags = llvm::Optional<std::vector<DiagWithFixIts>>;
-
   DocVersion Version = Contents.Version;
   Path FileStr = File.str();
   VFSTag Tag = std::move(TaggedFS.Tag);
 
-  auto Callback = [this, Version, FileStr, Tag](OptDiags Diags) {
-    if (!Diags)
-      return; // A new reparse was requested before this one completed.
-
+  auto Callback = [this, Version, FileStr,
+                   Tag](std::vector<DiagWithFixIts> Diags) {
     // We need to serialize access to resulting diagnostics to avoid calling
     // `onDiagnosticsReady` in the wrong order.
     std::lock_guard<std::mutex> DiagsLock(DiagnosticsMutex);
     DocVersion &LastReportedDiagsVersion = ReportedDiagnosticVersions[FileStr];
     // FIXME(ibiryukov): get rid of '<' comparison here. In the current
     // implementation diagnostics will not be reported after version counters'
     // overflow. This should not happen in practice, since DocVersion is a
     // 64-bit unsigned integer.
     if (Version < LastReportedDiagsVersion)
       return;
     LastReportedDiagsVersion = Version;
 
     DiagConsumer.onDiagnosticsReady(
-        FileStr, make_tagged(std::move(*Diags), std::move(Tag)));
+        FileStr, make_tagged(std::move(Diags), std::move(Tag)));
   };
 
   WorkScheduler.update(File,
                        ParseInputs{std::move(Command),
                                    std::move(TaggedFS.Value),
                                    std::move(*Contents.Draft)},
-                       std::move(Callback));
+                       WantDiags, std::move(Callback));
 }
 
 void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
   // FIXME: Do nothing for now. This will be used for indexing and potentially
   // invalidating other caches.
 }
 
 std::vector<std::pair<Path, std::size_t>>
 ClangdServer::getUsedBytesPerFile() const {
   return WorkScheduler.getUsedBytesPerFile();
 }
 
 LLVM_NODISCARD bool
 ClangdServer::blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds) {
   return WorkScheduler.blockUntilIdle(timeoutSeconds(TimeoutSeconds));
 }

clang-tools-extra/trunk/clangd/TUScheduler.h

struct InputsAndAST {
   ParsedAST *
 };
 
 struct InputsAndPreamble {
   const ParseInputs &Inputs;
   const PreambleData *Preamble;
 };
 
+/// Determines whether diagnostics should be generated for a file snapshot.
+enum class WantDiagnostics {
+  Yes,  /// Diagnostics must be generated for this snapshot.
+  No,   /// Diagnostics must not be generated for this snapshot.
+  Auto, /// Diagnostics must be generated for this snapshot or a subsequent one,
+        /// within a bounded amount of time.
+};
+
 /// Handles running tasks for ClangdServer and managing the resources (e.g.,
 /// preambles and ASTs) for opened files.
 /// TUScheduler is not thread-safe, only one thread should be providing updates
 /// and scheduling tasks.
 /// Callbacks are run on a threadpool and it's appropriate to do slow work in
 /// them. Each task has a name, used for tracing (should be UpperCamelCase).
 class TUScheduler {
 public:
   TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
               ASTParsedCallback ASTCallback);
   ~TUScheduler();
 
   /// Returns estimated memory usage for each of the currently open files.
   /// The order of results is unspecified.
   std::vector<std::pair<Path, std::size_t>> getUsedBytesPerFile() const;
 
   /// Schedule an update for \p File. Adds \p File to a list of tracked files if
   /// \p File was not part of it before.
   /// FIXME(ibiryukov): remove the callback from this function.
-  void update(PathRef File, ParseInputs Inputs,
-              UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
-                  OnUpdated);
+  void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD,
+              UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated);
 
   /// Remove \p File from the list of tracked files and schedule removal of its
   /// resources.
   void remove(PathRef File);
 
   /// Schedule an async read of the AST. \p Action will be called when AST is
   /// ready. The AST passed to \p Action refers to the version of \p File
   /// tracked at the time of the call, even if new updates are received before

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

public:
   /// The processing thread is spawned using \p Tasks. However, when \p Tasks
   /// is null, all requests will be processed on the calling thread
   /// synchronously instead. \p Barrier is acquired when processing each
   /// request, it is be used to limit the number of actively running threads.
   static ASTWorkerHandle Create(llvm::StringRef File, AsyncTaskRunner *Tasks,
                                 Semaphore &Barrier, CppFile AST);
   ~ASTWorker();
 
-  void update(ParseInputs Inputs,
-              UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
-                  OnUpdated);
+  void update(ParseInputs Inputs, WantDiagnostics,
+              UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated);
   void runWithAST(llvm::StringRef Name,
                   UniqueFunction<void(llvm::Expected<InputsAndAST>)> Action);
   bool blockUntilIdle(Deadline Timeout) const;
 
   std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
   std::size_t getUsedBytes() const;
 
 private:
   // Must be called exactly once on processing thread. Will return after
   // stop() is called on a separate thread and all pending requests are
   // processed.
   void run();
   /// Signal that run() should finish processing pending requests and exit.
   void stop();
   /// Adds a new task to the end of the request queue.
   void startTask(llvm::StringRef Name, UniqueFunction<void()> Task,
-                 bool isUpdate, llvm::Optional<CancellationFlag> CF);
+                 llvm::Optional<WantDiagnostics> UpdateType);
+  /// Should the first task in the queue be skipped instead of run?
+  bool shouldSkipHeadLocked() const;
 
   struct Request {
     UniqueFunction<void()> Action;
     std::string Name;
     Context Ctx;
+    llvm::Optional<WantDiagnostics> UpdateType;
   };
 
   std::string File;
   const bool RunSync;
   Semaphore &Barrier;
   // AST and FileInputs are only accessed on the processing thread from run().
   CppFile AST;
   // Inputs, corresponding to the current state of AST.
   ParseInputs FileInputs;
   // Guards members used by both TUScheduler and the worker thread.
   mutable std::mutex Mutex;
   std::shared_ptr<const PreambleData> LastBuiltPreamble; /* GUARDED_BY(Mutex) */
   // Result of getUsedBytes() after the last rebuild or read of AST.
   std::size_t LastASTSize; /* GUARDED_BY(Mutex) */
   // Set to true to signal run() to finish processing.
   bool Done;                           /* GUARDED_BY(Mutex) */
-  std::queue<Request> Requests;        /* GUARDED_BY(Mutex) */
-  // Only set when last request is an update. This allows us to cancel an update
-  // that was never read, if a subsequent update comes in.
-  llvm::Optional<CancellationFlag> LastUpdateCF; /* GUARDED_BY(Mutex) */
+  std::deque<Request> Requests;        /* GUARDED_BY(Mutex) */
   mutable std::condition_variable RequestsCV;
 };
 
 /// A smart-pointer-like class that points to an active ASTWorker.
 /// In destructor, signals to the underlying ASTWorker that no new requests will
 /// be sent and the processing loop may exit (after running all pending
 /// requests).
 class ASTWorkerHandle {

 #ifndef NDEBUG
   std::lock_guard<std::mutex> Lock(Mutex);
   assert(Done && "handle was not destroyed");
   assert(Requests.empty() && "unprocessed requests when destroying ASTWorker");
 #endif
 }
 
 void ASTWorker::update(
-    ParseInputs Inputs,
-    UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
-        OnUpdated) {
-  auto Task = [=](CancellationFlag CF, decltype(OnUpdated) OnUpdated) mutable {
-    if (CF.isCancelled()) {
-      OnUpdated(llvm::None);
-      return;
-    }
+    ParseInputs Inputs, WantDiagnostics WantDiags,
+    UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
+  auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
     FileInputs = Inputs;
     auto Diags = AST.rebuild(std::move(Inputs));
 
     {
       std::lock_guard<std::mutex> Lock(Mutex);
       if (AST.getPreamble())
         LastBuiltPreamble = AST.getPreamble();
       LastASTSize = AST.getUsedBytes();
     }
     // We want to report the diagnostics even if this update was cancelled.
     // It seems more useful than making the clients wait indefinitely if they
     // spam us with updates.
-    OnUpdated(std::move(Diags));
+    if (Diags && WantDiags != WantDiagnostics::No)
+      OnUpdated(std::move(*Diags));
   };
 
-  CancellationFlag UpdateCF;
-  startTask("Update", BindWithForward(Task, UpdateCF, std::move(OnUpdated)),
-            /*isUpdate=*/true, UpdateCF);
+  startTask("Update", BindWithForward(Task, std::move(OnUpdated)), WantDiags);
 }
 
 void ASTWorker::runWithAST(
     llvm::StringRef Name,
     UniqueFunction<void(llvm::Expected<InputsAndAST>)> Action) {
   auto Task = [=](decltype(Action) Action) {
     ParsedAST *ActualAST = AST.getAST();
     if (!ActualAST) {
       Action(llvm::make_error<llvm::StringError>("invalid AST",
                                                  llvm::errc::invalid_argument));
       return;
     }
     Action(InputsAndAST{FileInputs, *ActualAST});
 
     // Size of the AST might have changed after reads too, e.g. if some decls
     // were deserialized from preamble.
     std::lock_guard<std::mutex> Lock(Mutex);
     LastASTSize = ActualAST->getUsedBytes();
   };
 
   startTask(Name, BindWithForward(Task, std::move(Action)),
-            /*isUpdate=*/false, llvm::None);
+            /*UpdateType=*/llvm::None);
 }
 
 std::shared_ptr<const PreambleData>
 ASTWorker::getPossiblyStalePreamble() const {
   std::lock_guard<std::mutex> Lock(Mutex);
   return LastBuiltPreamble;
 }
 
 std::size_t ASTWorker::getUsedBytes() const {
   std::lock_guard<std::mutex> Lock(Mutex);
   return LastASTSize;
 }
 
 void ASTWorker::stop() {
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     assert(!Done && "stop() called twice");
     Done = true;
   }
   RequestsCV.notify_all();
 }
 
 void ASTWorker::startTask(llvm::StringRef Name, UniqueFunction<void()> Task,
-                          bool isUpdate, llvm::Optional<CancellationFlag> CF) {
-  assert(isUpdate == CF.hasValue() &&
-         "Only updates are expected to pass CancellationFlag");
-
+                          llvm::Optional<WantDiagnostics> UpdateType) {
   if (RunSync) {
     assert(!Done && "running a task after stop()");
     trace::Span Tracer(Name + ":" + llvm::sys::path::filename(File));
     Task();
     return;
   }
 
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     assert(!Done && "running a task after stop()");
-    if (isUpdate) {
-      if (!Requests.empty() && LastUpdateCF) {
-        // There were no reads for the last unprocessed update, let's cancel it
-        // to not waste time on it.
-        LastUpdateCF->cancel();
-      }
-      LastUpdateCF = std::move(*CF);
-    } else {
-      LastUpdateCF = llvm::None;
-    }
-    Requests.push({std::move(Task), Name, Context::current().clone()});
-  } // unlock Mutex.
+    Requests.push_back(
+        {std::move(Task), Name, Context::current().clone(), UpdateType});
   RequestsCV.notify_all();
 }
 
 void ASTWorker::run() {
   while (true) {
     Request Req;
     {
       std::unique_lock<std::mutex> Lock(Mutex);
       RequestsCV.wait(Lock, [&]() { return Done || !Requests.empty(); });
       if (Requests.empty()) {
         assert(Done);
         return;
       }
       // Even when Done is true, we finish processing all pending requests
       // before exiting the processing loop.
 
+      while (shouldSkipHeadLocked())
+        Requests.pop_front();
+      assert(!Requests.empty() && "skipped the whole queue");
       Req = std::move(Requests.front());
       // Leave it on the queue for now, so waiters don't see an empty queue.
     } // unlock Mutex
 
     {
       std::lock_guard<Semaphore> BarrierLock(Barrier);
       WithContext Guard(std::move(Req.Ctx));
       trace::Span Tracer(Req.Name);
       Req.Action();
     }
 
     {
       std::lock_guard<std::mutex> Lock(Mutex);
-      Requests.pop();
+      Requests.pop_front();
     }
     RequestsCV.notify_all();
   }
 }
 
+// Returns true if Requests.front() is a dead update that can be skipped.
+bool ASTWorker::shouldSkipHeadLocked() const {
+  assert(!Requests.empty());
+  auto Next = Requests.begin();
+  auto UpdateType = Next->UpdateType;
+  if (!UpdateType) // Only skip updates.
+    return false;
+  ++Next;
+  // An update is live if its AST might still be read.
+  // That is, if it's not immediately followed by another update.
+  if (Next == Requests.end() || !Next->UpdateType)
+    return false;
+  // The other way an update can be live is if its diagnostics might be used.
+  switch (*UpdateType) {
+  case WantDiagnostics::Yes:
+    return false; // Always used.
+  case WantDiagnostics::No:
+    return true; // Always dead.
+  case WantDiagnostics::Auto:
+    // Used unless followed by an update that generates diagnostics.
+    for (; Next != Requests.end(); ++Next)
+      if (Next->UpdateType == WantDiagnostics::Yes ||
+          Next->UpdateType == WantDiagnostics::Auto)
+        return true; // Prefer later diagnostics.
+    return false;
+  }
+}
+
 bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
   std::unique_lock<std::mutex> Lock(Mutex);
   return wait(Lock, RequestsCV, Timeout, [&] { return Requests.empty(); });
 }
 
 } // namespace
 
 unsigned getDefaultAsyncThreadsCount() {
if (!File.getValue()->Worker->blockUntilIdle(D))
       return false;
   if (PreambleTasks)
     if (!PreambleTasks->wait(D))
       return false;
   return true;
 }
 
 void TUScheduler::update(
-    PathRef File, ParseInputs Inputs,
-    UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
-        OnUpdated) {
+    PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags,
+    UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
   std::unique_ptr<FileData> &FD = Files[File];
   if (!FD) {
     // Create a new worker to process the AST-related tasks.
     ASTWorkerHandle Worker = ASTWorker::Create(
         File, WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier,
         CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback));
     FD = std::unique_ptr<FileData>(new FileData{Inputs, std::move(Worker)});
   } else {
     FD->Inputs = Inputs;
   }
-  FD->Worker->update(std::move(Inputs), std::move(OnUpdated));
+  FD->Worker->update(std::move(Inputs), WantDiags, std::move(OnUpdated));
 }
 
 void TUScheduler::remove(PathRef File) {
   bool Removed = Files.erase(File);
   if (!Removed)
     log("Trying to remove file from TUScheduler that is not tracked. File:" +
         File);
 }

clang-tools-extra/trunk/clangd/Threading.h

 //===--- ThreadPool.h --------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_H
 
 #include "Context.h"
 #include "Function.h"
 #include "llvm/ADT/Twine.h"
-#include <atomic>
 #include <cassert>
 #include <condition_variable>
 #include <memory>
 #include <mutex>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
-/// A shared boolean flag indicating if the computation was cancelled.
-/// Once cancelled, cannot be returned to the previous state.
-class CancellationFlag {
+/// A threadsafe flag that is initially clear.
+class Notification {
 public:
-  CancellationFlag();
-
-  void cancel() {
-    assert(WasCancelled && "the object was moved");
-    WasCancelled->store(true);
-  }
-
-  bool isCancelled() const {
-    assert(WasCancelled && "the object was moved");
-    return WasCancelled->load();
-  }
+  // Sets the flag. No-op if already set.
+  void notify();
+  // Blocks until flag is set.
+  void wait() const;
 
 private:
-  std::shared_ptr<std::atomic<bool>> WasCancelled;
+  bool Notified = false;
+  mutable std::condition_variable CV;
+  mutable std::mutex Mu;
 };
 
 /// Limits the number of threads that can acquire the lock at the same time.
 class Semaphore {
 public:
   Semaphore(std::size_t MaxLocks);
 
   void lock();

clang-tools-extra/trunk/clangd/Threading.cpp

 #include "Threading.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Threading.h"
 #include <thread>
 
 namespace clang {
 namespace clangd {
 
-CancellationFlag::CancellationFlag()
-    : WasCancelled(std::make_shared<std::atomic<bool>>(false)) {}
+void Notification::notify() {
+  {
+    std::lock_guard<std::mutex> Lock(Mu);
+    Notified = true;
+  }
+  CV.notify_all();
+}
+
+void Notification::wait() const {
+  std::unique_lock<std::mutex> Lock(Mu);
+  CV.wait(Lock, [this] { return Notified; });
+}
 
 Semaphore::Semaphore(std::size_t MaxLocks) : FreeSlots(MaxLocks) {}
 
 void Semaphore::lock() {
   std::unique_lock<std::mutex> Lock(Mutex);
   SlotsChanged.wait(Lock, [&]() { return FreeSlots > 0; });
   --FreeSlots;
 }

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

TUScheduler S(getDefaultAsyncThreadsCount(),
                 /*ASTParsedCallback=*/nullptr);
 
   auto Added = testPath("added.cpp");
   Files[Added] = "";
 
   auto Missing = testPath("missing.cpp");
   Files[Missing] = "";
 
-  S.update(Added, getInputs(Added, ""), ignoreUpdate);
+  S.update(Added, getInputs(Added, ""), WantDiagnostics::No, ignoreUpdate);
 
   // Assert each operation for missing file is an error (even if it's available
   // in VFS).
   S.runWithAST("", Missing, [&](llvm::Expected<InputsAndAST> AST) {
     ASSERT_FALSE(bool(AST));
     ignoreError(AST.takeError());
   });
   S.runWithPreamble("", Missing,
TEST_F(TUSchedulerTests, MissingFiles) {
   S.runWithPreamble("", Added, [&](llvm::Expected<InputsAndPreamble> Preamble) {
     ASSERT_FALSE(bool(Preamble));
     ignoreError(Preamble.takeError());
   });
   // remove() shouldn't crash on missing files.
   S.remove(Added);
 }
 
+TEST_F(TUSchedulerTests, WantDiagnostics) {
+  std::atomic<int> CallbackCount(0);
+  {
+    TUScheduler S(getDefaultAsyncThreadsCount(),
+                  /*StorePreamblesInMemory=*/true,
+                  /*ASTParsedCallback=*/nullptr);
+    auto Path = testPath("foo.cpp");
+
+    // To avoid a racy test, don't allow tasks to actualy run on the worker
+    // thread until we've scheduled them all.
+    Notification Ready;
+    S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
+             [&](std::vector<DiagWithFixIts>) { Ready.wait(); });
+
+    S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
+             [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+    S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
+             [&](std::vector<DiagWithFixIts> Diags) {
+               ADD_FAILURE() << "auto should have been cancelled by auto";
+             });
+    S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
+             [&](std::vector<DiagWithFixIts> Diags) {
+               ADD_FAILURE() << "no diags should not be called back";
+             });
+    S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
+             [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+    Ready.notify();
+  }
+  EXPECT_EQ(2, CallbackCount);
+}
+
 TEST_F(TUSchedulerTests, ManyUpdates) {
   const int FilesCount = 3;
   const int UpdatesPerFile = 10;
 
   std::mutex Mut;
   int TotalASTReads = 0;
   int TotalPreambleReads = 0;
   int TotalUpdates = 0;
for (int FileI = 0; FileI < FilesCount; ++FileI) {
       for (int UpdateI = 0; UpdateI < UpdatesPerFile; ++UpdateI) {
         auto Contents = AllContents[(FileI + UpdateI) % AllContentsSize];
 
         auto File = Files[FileI];
         auto Inputs = getInputs(File, Contents.str());
 
         {
           WithContextValue WithNonce(NonceKey, ++Nonce);
-          S.update(File, Inputs,
+          S.update(File, Inputs, WantDiagnostics::Auto,
                    [Nonce, &Mut, &TotalUpdates](
                        llvm::Optional<std::vector<DiagWithFixIts>> Diags) {
                      EXPECT_THAT(Context::current().get(NonceKey),
                                  Pointee(Nonce));
 
                      std::lock_guard<std::mutex> Lock(Mut);
                      ++TotalUpdates;
                    });