[PATCH] D88172: [clangd] Extract common file-caching logic from ConfigProvider.

Wed Nov 25 02:53:26 PST 2020

sammccall marked an inline comment as done.
sammccall added inline comments.

================
Comment at: clang-tools-extra/clangd/support/FileCache.cpp:59
+  // If the modified-time and size match, assume the content does too.
+  if (Size == Stat->getSize() &&
+      ModifiedTime == Stat->getLastModificationTime())
----------------
njames93 wrote:
> Is tracking the size really that important?
> Modified time should be good enough. If someone modifies the file but ensures the mtime doesnt change you have to think there is a reason for that. Also a modification could preserve file size, in which case that wouldn't track anything.
If the content changes without changing mtime, and we don't detect it using size, we're stuck with an invalid cache until it changes again (which could be forever).

> you have to think there is a reason for that

My best guess is if the FS has a low resolution (e.g. FAT32, though hopefully people aren't putting config files on USB sticks...) or the file was written twice e.g. by a script fast enough to beat the timer. In both cases I'd want the changes to be revealed.
What's the reason you're thinking of?

> Also a modification could preserve file size, in which case that wouldn't track anything
Yup, I don't have a good answer for that case. Hopefully it's very rare. I don't think we should let the perfect be the enemy of the good.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88172