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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/TUScheduler.cpp:338
+      Barrier(Barrier), Done(false) {
+  FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+}
----------------
ilya-biryukov wrote:
> The command is filled with a fallback by `ClangdServer`, right? Why do we need to repeat this here?
`ASTWorker::FileInputs` is not set until ASTWorker runs update and gets compile command. Before that, `FileInputs` contains empty compile command that can be used by `runWithPreamble`/`runWithAST` (afaict). Initializing it to fallback command seems to be a better alternative.


================
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.
----------------
ilya-biryukov wrote:
> 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).
If `CDB.getCompileCommand` failed, we would use `Inputs.CompileCommand` which should be the fallback command set in ClangdServer. It might be a good idea to use the old command, but it doesn't seem to be in the scope of this patch. Added a `FIXME`.


================
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+  return FileInputs.CompileCommand;
+}
----------------
ilya-biryukov wrote:
> 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)
It looks like we could also guard `FileInputs`? Adding another variable seems to make the state more complicated. 


================
Comment at: clangd/TUScheduler.cpp:835
 
 void TUScheduler::update(PathRef File, ParseInputs Inputs,
                          WantDiagnostics WantDiags) {
----------------
ilya-biryukov wrote:
> 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`.
Added commen. Unfortunately, `ParseInputs` contains other things like `Index` and `Opts`, so we couldn't replace it with FS and Contents.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607