[PATCH] D77847: [clangd] Rebuild dependent files when a header is saved.

Sat Apr 11 06:23:28 PDT 2020

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM!

In D77847#1975940 <https://reviews.llvm.org/D77847#1975940>, @sammccall wrote:

> In D77847#1974126 <https://reviews.llvm.org/D77847#1974126>, @kadircet wrote:
>
> > should we also have a unittest for checking ast caching works as expected?
>
>
> TUSchedulerTests has `NoopOnEmptyChanges` which I think tests exactly this, and `EvictedAST` which checks it's sensitive to the cache.

Right, but none of them checks exactly this behaviour.  The `NoopOnEmptyChanges` test only ensures we don't publish diagnostics again
and `EvictedAST` test actually asserts on the opposite, it checks the last updated file is "always" cached. Maybe extending `EvictedAST`
test with a "noop update" might be a better fit. Up to you though.

> I don't think testing it again indirectly at this level makes is a good tradeoff, especially if we need subtle assertions via the memory usage.

To be clear I wasn't suggesting a test in ClangdLSPServer layer, but rather on ClangdServer layer. but as mentioned above, tuscheduler might
be a better level.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77847/new/

https://reviews.llvm.org/D77847