ioeric marked an inline comment as done.
ioeric added inline comments.
================
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > 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.
> Unfortunately we can't share `Inputs.FS` safely as the vfs implementations
> are not thread-safe.
It seems to me that the behavior for `FS` hasn't change in this patch. And
`FileInputs` (incl. `Inputs.FS`) has always been available/shared across worker
threads. We don't seem to have systematic way to prevent raciness before this
patch, and it would be good to have it somehow, but it's probably out of the
scope.
Maybe I'm missing something or misinterpreting. Could you elaborate?
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
