[PATCH] D102761: [clangd] New ParsingCallback for semantics changes

[Kadir Cetinkaya via Phabricator via cfe-commits]

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