[PATCH] D136651: [Clang] Give Clang the ability to use a shared stat cache

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.

Re-added a few of my minor comments that I think got lost on a specific version of the diff. Otherwise LGTM.



================
Comment at: llvm/include/llvm/Support/StatCacheFileSystem.h:23
+/// A ProxyFileSystem using cached information for status() rather than going to
+/// the real filesystem.
+///
----------------
s/real/underlying/ - it could be wrapping any filesystem in principle.


================
Comment at: llvm/include/llvm/Support/StatCacheFileSystem.h:26
+/// When dealing with a huge tree of (mostly) immutable filesystem content
+/// like and SDK, it can be very costly to ask the underlying filesystem for
+/// `stat` data. Even when caching the `stat`s internally, having many
----------------
typo: "and SDK" -> "an SDK"


================
Comment at: llvm/include/llvm/Support/StatCacheFileSystem.h:51
+  static Expected<IntrusiveRefCntPtr<StatCacheFileSystem>>
+  create(std::unique_ptr<llvm::MemoryBuffer> &&CacheBuffer,
+         StringRef CacheFilename, IntrusiveRefCntPtr<FileSystem> FS);
----------------
Nit: It's unusal to see unique_ptr && instead of just unique_ptr. Is this needed? Same for the constructor.


================
Comment at: llvm/include/llvm/Support/StatCacheFileSystem.h:52
+  create(std::unique_ptr<llvm::MemoryBuffer> &&CacheBuffer,
+         StringRef CacheFilename, IntrusiveRefCntPtr<FileSystem> FS);
+
----------------
Is CacheFilename different from CacheBuffer->getBufferIdentifier()?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136651