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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/TUScheduler.cpp:338
+      Barrier(Barrier), Done(false) {
+  FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+}
----------------
ioeric wrote:
> 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.
That makes sense. Could we remove the `getFallbackCommand` call from the `ClangdServer::addDocument`?
Let's make the `TUScheduler` fully responsible for the compile command.


================
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.
----------------
ioeric wrote:
> 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`.
Oh, I mixed `Inputs` and `FileInputs` in my head.
It feels reasonable to completely move the handling of compile command to `TUScheduler`, see the other comment at the constructor of `ASTWorker`.


================
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+  return FileInputs.CompileCommand;
+}
----------------
ioeric wrote:
> 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. 
`FileInputs` are currently accessed without locks in a bunch of places and it'd be nice to keep it that way (the alternative is more complicated code).
Doing the same thing we do for preamble looks like the best alternative here.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607