bnbarham added a comment.
Mostly just skimmed so far, will hopefully have some time to look at this more
tomorrow. Out of interest, do you have any performance numbers using this
change? I assume this mainly impacts implicit modules (since I suspect we'd
also be opening the file as well anyway), is that true?
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:5115-5130
+ for (const auto &File : CI.getHeaderSearchOpts().VFSStatCacheFiles) {
+ llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buffer =
+ llvm::MemoryBuffer::getFile(File);
+ if (!Buffer) {
+ Diags.Report(diag::err_missing_vfs_stat_cache_file) << File;
+ continue;
+ }
----------------
IMO VFS overlays being modeled like this is a mistake and makes
reasoning/testing about them fairly difficult. I have a PR up
https://reviews.llvm.org/D121423 to change `OverlayFileSystem` to make more
sense and be a real overlay rather than what it is now. If I finish that one
off, how would you feel about changing the behavior of `StatCacheFileSystem` to
just immediately error if it doesn't contain the file, rather than proxy (and
thus chain) as it does now?
So for multiple files we'd then have:
- OverlayFileSystem
- StatCacheFileSystem
- StatCacheFileSystem
- RealFileSystem
Then any non-stat or exists call would return
`llvm::errc::no_such_file_or_directory` and then the next FS would be used
instead.
I don't think this *really* matters for `StatCacheFileSystem`, so I'm fine if
you'd rather not wait for me to change `OverlayFileSystem`. I can make the
changes myself after getting my PR in.
================
Comment at: clang/tools/clang-stat-cache/clang-stat-cache.cpp:143
+// There is no cross-platform way to implement a validity check. If this
+// platofrm doesn't support it, just consider the cache constents always
+// valid. When that's the case, the tool running cache generation needs
----------------
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2958
+ // cache prefix. It could be that the file doesn't exist, or the spelling
+ // the pathis different. The canonicalization that the call to
remove_dots()
+ // does leaves only '..' with symlinks as a source of confusion. If the
path
----------------
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2959-2960
+ // the pathis different. The canonicalization that the call to
remove_dots()
+ // does leaves only '..' with symlinks as a source of confusion. If the
path
+ // does not contain '..' we can safely say it doesn't exist.
+ if (std::find(sys::path::begin(SuffixPath), sys::path::end(SuffixPath),
----------------
This sentence is a little confusing to me. `remove_dots` just leaves `..`
unless you pass `remove_dot_dot` (but it doesn't do any checking). IMO just the
`If the path does not contain '..' we can safely say it doesn't exist.` is
enough.
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2961
+ // does not contain '..' we can safely say it doesn't exist.
+ if (std::find(sys::path::begin(SuffixPath), sys::path::end(SuffixPath),
+ "..") == sys::path::end(SuffixPath)) {
----------------
FWIW `StringRef` has a `contains`
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
