kadircet added a comment. > It seems like a lit test for this would be terrible. A ClangdServer one > should be possible, but I can't quite wrap my head around how to write it. > (Delivering the PreambleData as a param would make the test easier, you could > assert that the preamble version was > any previously notified preamble & > also > any preamble previously obtained via runWithPreamble.)
I also put some thought into it, but couldn't figure out a way that wouldn't be flaky. we can write a test that might catch a regression, but it won't catch all of them for sure unless we somehow block the execution of threads. I tried to find some locking solution that wouldn't depend too much on the TUScheduler's implementation details but couldn't find any, and eventually gave up :/. I can include a test that ensures `runWithAST` always sees a new preamble after `onPreamblePublished`, but as mentioned it is going to be a flaky at best. WDYT? ================ Comment at: clang-tools-extra/clangd/TUScheduler.h:175 + /// on the file are guranteed to see new semantics after the callback. + virtual void onSemanticsMaybeChanged(PathRef File) {} }; ---------------- sammccall wrote: > This seems a little vague/abstract for TUScheduler. > The "caller" (i.e. ClangdServer impl) needs to know about preambles etc, c.f. > onPreambleAST in this same interface. > > I'd suggest `onPreambleActive` or `onPreamblePublished` or something like > that. > (Passing the PreambleData struct as a param would also hint nicely at the > distinction vs onPreambleAST but is maybe too cute if we're not actually > going to use it...) going with `onPreamblePublished`, since `Active` might mean it wasn't previously before (e.g. first preamble for a TU). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102761/new/ https://reviews.llvm.org/D102761 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits