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

Reply via email to