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

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 222474.
kousikk added a comment.

Avoid the extra stat() call


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68193

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -25,6 +25,8 @@
   llvm::ErrorOr<llvm::vfs::Status> Stat = (*MaybeFile)->status();
   if (!Stat)
     return Stat.getError();
+  if (Stat->isDirectory())
+    return std::make_error_code(std::errc::is_a_directory);
 
   llvm::vfs::File &F = **MaybeFile;
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
@@ -233,15 +235,14 @@
     CachedFileSystemEntry &CacheEntry = SharedCacheEntry.Value;
 
     if (!CacheEntry.isValid()) {
-      llvm::vfs::FileSystem &FS = getUnderlyingFS();
-      auto MaybeStatus = FS.status(Filename);
-
-      if (MaybeStatus && MaybeStatus->isDirectory())
-        return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(
-          std::make_error_code(std::errc::is_a_directory));
-
       CacheEntry = CachedFileSystemEntry::createFileEntry(
-          Filename, FS, !KeepOriginalSource);
+        Filename, getUnderlyingFS(), !KeepOriginalSource);
+      if (CacheEntry.getError() == std::errc::is_a_directory) {
+        // Reset the CacheEntry to avoid setting an error entry in the
+        // cache for the given directory path.
+        CacheEntry = CachedFileSystemEntry();
+        return 
llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(std::errc::is_a_directory);
+      }
     }
 
     Result = &CacheEntry;
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===================================================================
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -80,6 +80,11 @@
     return MaybeStat->getName();
   }
 
+  std::error_code getError() const {
+    assert(isValid() && "not initialized");
+    return MaybeStat.getError();
+  }
+
   /// Return the mapping between location -> distance that is used to speed up
   /// the block skipping in the preprocessor.
   const PreprocessorSkippedRangeMapping &getPPSkippedRangeMapping() const {



Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -25,6 +25,8 @@
   llvm::ErrorOr<llvm::vfs::Status> Stat = (*MaybeFile)->status();
   if (!Stat)
     return Stat.getError();
+  if (Stat->isDirectory())
+    return std::make_error_code(std::errc::is_a_directory);
 
   llvm::vfs::File &F = **MaybeFile;
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
@@ -233,15 +235,14 @@
     CachedFileSystemEntry &CacheEntry = SharedCacheEntry.Value;
 
     if (!CacheEntry.isValid()) {
-      llvm::vfs::FileSystem &FS = getUnderlyingFS();
-      auto MaybeStatus = FS.status(Filename);
-
-      if (MaybeStatus && MaybeStatus->isDirectory())
-        return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(
-          std::make_error_code(std::errc::is_a_directory));
-
       CacheEntry = CachedFileSystemEntry::createFileEntry(
-          Filename, FS, !KeepOriginalSource);
+        Filename, getUnderlyingFS(), !KeepOriginalSource);
+      if (CacheEntry.getError() == std::errc::is_a_directory) {
+        // Reset the CacheEntry to avoid setting an error entry in the
+        // cache for the given directory path.
+        CacheEntry = CachedFileSystemEntry();
+        return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(std::errc::is_a_directory);
+      }
     }
 
     Result = &CacheEntry;
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===================================================================
--- clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -80,6 +80,11 @@
     return MaybeStat->getName();
   }
 
+  std::error_code getError() const {
+    assert(isValid() && "not initialized");
+    return MaybeStat.getError();
+  }
+
   /// Return the mapping between location -> distance that is used to speed up
   /// the block skipping in the preprocessor.
   const PreprocessorSkippedRangeMapping &getPPSkippedRangeMapping() const {

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits