dexonsmith added inline comments.
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:161-162
+ const StringRef RawFilename) {
+ llvm::SmallString<256> Filename;
+ llvm::sys::path::native(RawFilename, Filename);
+
----------------
I'm a bit nervous about the impact of modifying the input filename on Windows
before passing it into other APIs. This could change behaviour of lower layers
of the VFS (since they'll see a different filename than when
DependencyScanningWOrkerFileSystem is NOT on top of them).
Can we restrict this just to what's passed to IgnoredFiles? (Maybe add
`shouldIgnore()` API, which returns `false` if the set is empty, and then
locally converts to native and checks for membership...)
It also seems wasteful to be calling `sys::path::native` and the memcpy all the
time, when usually it has no effect. Have you checked whether this affects
performance of scanning something big?
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:171-172
bool KeepOriginalSource = IgnoredFiles.count(Filename) ||
!shouldMinimize(Filename);
DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
----------------
Looking at this, makes me wonder if this is just fixing a specific instance of
a more general problem.
Maybe `IgnoredFiles` should be a set of `FileEntry`s instead of `StringRef`s...
but that'd create a different performance bottleneck when the set is big, since
creating the FileEntrys would be expensive. We'd want the FileEntry lookup to
be globally cached / etc. -- and FileManager isn't quite safe to use globally.
Do you think IgnoredFiles as-is will work well enough for where it'll be used
for PCH? Or do we need to catch headers referenced in two different ways
somehow?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106064/new/
https://reviews.llvm.org/D106064
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
