[PATCH] D106064: [clang][deps] Normalize paths in minimizing file system

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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