ilya-biryukov added a comment. This is long overdue
================ Comment at: clangd/TUScheduler.cpp:338 + Barrier(Barrier), Done(false) { + FileInputs.CompileCommand = CDB.getFallbackCommand(FileName); +} ---------------- The command is filled with a fallback by `ClangdServer`, right? Why do we need to repeat this here? ================ Comment at: clangd/TUScheduler.cpp:358 + // current implementation. + if (auto Cmd = CDB.getCompileCommand(FileName)) + Inputs.CompileCommand = *Cmd; ---------------- Could you document that we initially start with a fallback, so update here? ================ 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. ---------------- 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). ================ Comment at: clangd/TUScheduler.cpp:552 +const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const { + return FileInputs.CompileCommand; +} ---------------- 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) ================ Comment at: clangd/TUScheduler.cpp:835 void TUScheduler::update(PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags) { ---------------- 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`. 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