ioeric added inline comments.
================
Comment at: clangd/TUScheduler.cpp:338
+ Barrier(Barrier), Done(false) {
+ FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+}
----------------
ilya-biryukov wrote:
> 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.
Sounds good.
================
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
----------------
ilya-biryukov wrote:
> 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.
The reason I think it would be easier to guard `FileInputs` with mutex instead
of pulling a new variable is that CompileCommand is usually used along with
other fields in `FileInputs`. I think we could manage this with relatively few
changes. Please take a look at the new changes.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
