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