[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

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

dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:21-22
   // Load the file and its content from the file system.
   llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> MaybeFile =
       FS.openFileForRead(Filename);
   if (!MaybeFile)
----------------
Note that the file is opened here.


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:236-237
     if (!CacheEntry.isValid()) {
+      llvm::vfs::FileSystem &FS = getUnderlyingFS();
+      auto MaybeStatus = FS.status(Filename);
+
----------------
It seems wasteful to do an extra stat here when the file is already open in `createFileEntry`.

Can you instead change `createFileEntry` to return `std::errc::is_a_directory` as appropriate to avoid the extra filesystem access?  (Is it possible that it's already doing that, and you just need to check for that?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193