kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land.
thanks, lgtm! ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:180 + for (auto &Mod : *Modules) + Mod.blockUntilIdle(Deadline::infinity()); + } ---------------- sammccall wrote: > kadircet wrote: > > why is our contract saying that just calling `stop` is not enough? > > i think clangdserver should just signal shutdown to modules, and our > > contract should say that server facilities will be undefined from this > > point forward. > > that way modules accessing the facilities, could block stop until they are > > done, and never make use of it afterwards? it'll make modules a little more > > complicated, at the very least they would need some stricter control > > whenever they are accessing facilities, but I think it is worth for keeping > > clangdserver shutdown cleaner. wdyt? > We need Module::blockUntilIdle anyway for tests and stuff. And in practice > requesting shutdown of background work doesn't naturally block until that > work is complete IME. > > So we could ask every module to call blockUntilIdle() in stop(), or make > stop() non-virtual and have it wrap requestStop()+blockUntilIdle(). > > Neither really seems simpler/cleaner to me overall, and it means modules shut > down in serial instead of parallel. > > We need Module::blockUntilIdle anyway for tests and stuff i was trying to ensure it is *only* used for tests, but i suppose you are right, in practice making `stop` block until in-flight tasks finishes vs stop+blockuntilidle is likely to have same effect, with the latter having the benefit of triggering shut down in parallel. so SGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96755/new/ https://reviews.llvm.org/D96755 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
