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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
