[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This is long overdue



================
Comment at: clangd/TUScheduler.cpp:338
+      Barrier(Barrier), Done(false) {
+  FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+}
----------------
The command is filled with a fallback by `ClangdServer`, right? Why do we need to repeat this here?


================
Comment at: clangd/TUScheduler.cpp:358
+    // current implementation.
+    if (auto Cmd = CDB.getCompileCommand(FileName))
+      Inputs.CompileCommand = *Cmd;
----------------
Could you document that we initially start with a fallback, so update here?


================
Comment at: clangd/TUScheduler.cpp:359
+    if (auto Cmd = CDB.getCompileCommand(FileName))
+      Inputs.CompileCommand = *Cmd;
     // Will be used to check if we can avoid rebuilding the AST.
----------------
Maybe update the heuristic field in the current compile command if its empty to indicate we're using an older command?
That might happen if a previous call to `getCompileCommand` succeeded (so the stored command does not have a heuristic set).


================
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+  return FileInputs.CompileCommand;
+}
----------------
This is racy, `FileInputs` is only accessed from a worker thread now.
I'm afraid we'll need a separate variable with a lock around it (could reuse one lock for preamble and compile command, probably)


================
Comment at: clangd/TUScheduler.cpp:835
 
 void TUScheduler::update(PathRef File, ParseInputs Inputs,
                          WantDiagnostics WantDiags) {
----------------
We should add a comment that compile command in inputs is ignored.
IMO even better is to accept an `FS` and `Contents` instead of `ParseInputs`.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60607/new/

https://reviews.llvm.org/D60607