sammccall added a comment. Looks pretty good now, thanks! Test needs to be more precise (doesn't actually test the behavior at present, I think).
================ Comment at: clang-tools-extra/clangd/Compiler.h:41 bool SuggestMissingIncludes = false; + bool ForceRebuild = false; }; ---------------- Hmm, on thinking further I do think this belongs in ParseInputs next to Contents or CompileCommands, as these things are things that affect the parse but *don't* invalidate caches. ================ Comment at: clang-tools-extra/clangd/Compiler.h:41 bool SuggestMissingIncludes = false; + bool ForceRebuild = false; }; ---------------- sammccall wrote: > Hmm, on thinking further I do think this belongs in ParseInputs next to > Contents or CompileCommands, as these things are things that affect the parse > but *don't* invalidate caches. this is a good place for a comment explaining what this does specifically (prevents reuse of cached preamble/ast) ================ Comment at: clang-tools-extra/clangd/Protocol.h:660 + /// This is a clangd extension. + bool forceRebuild; }; ---------------- `= false`, and remove "disabled by default" from comment ================ Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:619 +TEST_F(TUSchedulerTests, ForceRebuild) { + TUScheduler S(CDB, optsForTest(), captureDiags()); ---------------- This test doesn't actually verify that the preamble was rebuilt, just that the AST was. Checking the actual diagnostics would do that. ================ Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:631 + // Return value indicates if the updated callback was received. + auto DoUpdate = [&](std::string Contents, bool ForceRebuild) -> bool { + std::atomic<bool> Updated(false); ---------------- I don't think this abstraction is useful since you just have 3 calls and two of them need the actual diagnostics. You could have diag callbacks inline easily enough: ``` vector<vector<Diag>> diags; update(..., [] { diags.push_back(...) }) update(..., [] { ADD_FAILURE(...) }) update(..., [] { diags.push_back(...) }) blockUntilIdle(); EXPECT_THAT(diags, ElementsAre( /* First */ElementsAre(...missing header...), /* Last */IsEmpty()); ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73916/new/ https://reviews.llvm.org/D73916 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits