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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
