jansvoboda11 added inline comments.
================ Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:132-134 + /// Non-owning pointer to the file contents. This can be shared between + /// multiple entries (e.g. between a symlink and its target). + std::atomic<CachedFileContents *> ContentsAccess; ---------------- dexonsmith wrote: > This comment is out of date, since there's a 1:1 correspondence between > CachedFileSystemEntry and CachedFileContents since they're both looked up by > UID. > > Also, `ContentsAccess` is never modified after construction so it can just be > a raw pointer. > > Outlining the allocation of `CachedFileContents` might still make sense, > since it's big and stat failures (with no content) are probably the common > case due to header search patterns... only if we actually create these for > stat failures though. Might be worth a comment saying why it's outlined. > Maybe it should even be delayed to a follow-up commit to simplify this patch, > since now that CachedFileSystemEntry is per-UID it doesn't seem to be > required here... but the fields probably need to be made `mutable` regardless > somehow so it doesn't seem like a ton of churn. I've updated the comment and changed `ContentsAccess` to a raw pointer. I agree outlining `CachedFileContents` in a follow-up patch would be cleaner, but I don't have much time to spare on prettifying git history at this moment unfortunately. ================ Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:160-161 + + /// Map from unique IDs to cached contents. + llvm::DenseMap<llvm::sys::fs::UniqueID, CachedFileContents *> ContentsCache; + ---------------- dexonsmith wrote: > I think this map can be deleted, since it's not actually used to > deduplicate/share anything that `EntriesByUID` doesn't handle. Yeah, that's right. Removed this in the latest revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114966/new/ https://reviews.llvm.org/D114966 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
