[FileSystemStatCache] Return std::error_code from stat cache methods

[harlanhaskins]

Summary

Previously, we would return true/false signifying if the cache/lookup
succeeded or failed. Instead, provide clients with the underlying error
that was thrown while attempting to look up in the cache.

Since clang::FileManager doesn't make use of this information, it discards the
error that's received and casts away to bool.

[benlangmuir]

LGTM, but I'd appreciate someone who has worked on this more recently taking a look.

[arphaman]

LGTM, please mark it as NFC

include/clang/Basic/FileSystemStatCache.h

 /// system calls, which is used by precompiled and pretokenized headers to
 /// improve performance.
 class FileSystemStatCache {
   virtual void anchor();
 
 public:
   virtual ~FileSystemStatCache() = default;
 
-  enum LookupResult {
-    /// We know the file exists and its cached stat data.
-    CacheExists,
-
-    /// We know that the file doesn't exist.
-    CacheMissing
-  };
-
   /// Get the 'stat' information for the specified path, using the cache
   /// to accelerate it if possible.
   ///
   /// \returns \c true if the path does not exist or \c false if it exists.
   ///
   /// If isFile is true, then this lookup should only return success for files
   /// (not directories).  If it is false this lookup should only return
   /// success for directories (not files).  On a successful file lookup, the
   /// implementation can optionally fill in \p F with a valid \p File object and
   /// the client guarantees that it will close it.
-  static bool get(StringRef Path, llvm::vfs::Status &Status, bool isFile,
-                  std::unique_ptr<llvm::vfs::File> *F,
-                  FileSystemStatCache *Cache, llvm::vfs::FileSystem &FS);
+  static std::error_code
+  get(StringRef Path, llvm::vfs::Status &Status, bool isFile,
 
 protected:
   // FIXME: The pointer here is a non-owning/optional reference to the
   // unique_ptr. Optional<unique_ptr<vfs::File>&> might be nicer, but
   // Optional needs some work to support references so this isn't possible yet.
-  virtual LookupResult getStat(StringRef Path, llvm::vfs::Status &Status,
+  virtual std::error_code getStat(StringRef Path, llvm::vfs::Status &Status,
                                   bool isFile,
                                   std::unique_ptr<llvm::vfs::File> *F,
                                   llvm::vfs::FileSystem &FS) = 0;
 };
 
 /// A stat "cache" that can be used by FileManager to keep
 /// track of the results of stat() calls that occur throughout the
 /// execution of the front end.
 class MemorizeStatCalls : public FileSystemStatCache {
 public:
   /// The set of stat() calls that have been seen.
   llvm::StringMap<llvm::vfs::Status, llvm::BumpPtrAllocator> StatCalls;
 
   using iterator =
       llvm::StringMap<llvm::vfs::Status,
                       llvm::BumpPtrAllocator>::const_iterator;
 
   iterator begin() const { return StatCalls.begin(); }
   iterator end() const { return StatCalls.end(); }
 
-  LookupResult getStat(StringRef Path, llvm::vfs::Status &Status, bool isFile,
+  std::error_code getStat(StringRef Path, llvm::vfs::Status &Status,
+                          bool isFile,
                           std::unique_ptr<llvm::vfs::File> *F,
                           llvm::vfs::FileSystem &FS) override;
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_BASIC_FILESYSTEMSTATCACHE_H

lib/Basic/FileManager.cpp

 /// false if it's an existent real file.  If FileDescriptor is NULL,
 /// do directory look-up instead of file look-up.
 bool FileManager::getStatValue(StringRef Path, llvm::vfs::Status &Status,
                                bool isFile,
                                std::unique_ptr<llvm::vfs::File> *F) {
   // FIXME: FileSystemOpts shouldn't be passed in here, all paths should be
   // absolute!
   if (FileSystemOpts.WorkingDir.empty())
-    return FileSystemStatCache::get(Path, Status, isFile, F,StatCache.get(), *FS);
+    return bool(FileSystemStatCache::get(Path, Status, isFile, F,
+                                         StatCache.get(), *FS));
 
   SmallString<128> FilePath(Path);
   FixupRelativePath(FilePath);
 
-  return FileSystemStatCache::get(FilePath.c_str(), Status, isFile, F,
-                                  StatCache.get(), *FS);
+  return bool(FileSystemStatCache::get(FilePath.c_str(), Status, isFile, F,
+                                       StatCache.get(), *FS));
 }
 
 bool FileManager::getNoncachedStatValue(StringRef Path,
                                         llvm::vfs::Status &Result) {
   SmallString<128> FilePath(Path);
   FixupRelativePath(FilePath);
 
   llvm::ErrorOr<llvm::vfs::Status> S = FS->status(FilePath.c_str());

lib/Basic/FileSystemStatCache.cpp

 /// path, using the cache to accelerate it if possible.  This returns true if
 /// the path does not exist or false if it exists.
 ///
 /// If isFile is true, then this lookup should only return success for files
 /// (not directories).  If it is false this lookup should only return
 /// success for directories (not files).  On a successful file lookup, the
 /// implementation can optionally fill in FileDescriptor with a valid
 /// descriptor and the client guarantees that it will close it.
-bool FileSystemStatCache::get(StringRef Path, llvm::vfs::Status &Status,
-                              bool isFile,
-                              std::unique_ptr<llvm::vfs::File> *F,
-                              FileSystemStatCache *Cache,
-                              llvm::vfs::FileSystem &FS) {
-  LookupResult R;
+std::error_code
+FileSystemStatCache::get(StringRef Path, llvm::vfs::Status &Status,
+                         bool isFile, std::unique_ptr<llvm::vfs::File> *F,
   bool isForDir = !isFile;
+  std::error_code RetCode;
 
   // If we have a cache, use it to resolve the stat query.
   if (Cache)
-    R = Cache->getStat(Path, Status, isFile, F, FS);
+    RetCode = Cache->getStat(Path, Status, isFile, F, FS);
   else if (isForDir || !F) {
     // If this is a directory or a file descriptor is not needed and we have
     // no cache, just go to the file system.
     llvm::ErrorOr<llvm::vfs::Status> StatusOrErr = FS.status(Path);
     if (!StatusOrErr) {
-      R = CacheMissing;
+      RetCode = StatusOrErr.getError();
     } else {
-      R = CacheExists;
       Status = *StatusOrErr;
     }
   } else {
     // Otherwise, we have to go to the filesystem.  We can always just use
     // 'stat' here, but (for files) the client is asking whether the file exists
     // because it wants to turn around and *open* it.  It is more efficient to
     // do "open+fstat" on success than it is to do "stat+open".
     //
     // Because of this, check to see if the file exists with 'open'.  If the
     // open succeeds, use fstat to get the stat info.
     auto OwnedFile = FS.openFileForRead(Path);
 
     if (!OwnedFile) {
       // If the open fails, our "stat" fails.
-      R = CacheMissing;
+      RetCode = OwnedFile.getError();
     } else {
       // Otherwise, the open succeeded.  Do an fstat to get the information
       // about the file.  We'll end up returning the open file descriptor to the
       // client to do what they please with it.
       llvm::ErrorOr<llvm::vfs::Status> StatusOrErr = (*OwnedFile)->status();
       if (StatusOrErr) {
-        R = CacheExists;
         Status = *StatusOrErr;
         *F = std::move(*OwnedFile);
       } else {
         // fstat rarely fails.  If it does, claim the initial open didn't
         // succeed.
-        R = CacheMissing;
         *F = nullptr;
+        RetCode = StatusOrErr.getError();
       }
     }
   }
 
   // If the path doesn't exist, return failure.
-  if (R == CacheMissing) return true;
+  if (RetCode)
+    return RetCode;
 
   // If the path exists, make sure that its "directoryness" matches the clients
   // demands.
   if (Status.isDirectory() != isForDir) {
     // If not, close the file if opened.
     if (F)
       *F = nullptr;
-
-    return true;
+    return std::make_error_code(
+        Status.isDirectory() ?
+            std::errc::is_a_directory : std::errc::not_a_directory);
   }
 
-  return false;
+  return std::error_code();
 }
 
-MemorizeStatCalls::LookupResult
+std::error_code
 MemorizeStatCalls::getStat(StringRef Path, llvm::vfs::Status &Status,
                            bool isFile,
                            std::unique_ptr<llvm::vfs::File> *F,
                            llvm::vfs::FileSystem &FS) {
-  if (get(Path, Status, isFile, F, nullptr, FS)) {
+  auto err = get(Path, Status, isFile, F, nullptr, FS);
+  if (err) {
     // Do not cache failed stats, it is easy to construct common inconsistent
     // situations if we do, and they are not important for PCH performance
     // (which currently only needs the stats to construct the initial
     // FileManager entries).
-    return CacheMissing;
+    return err;
   }
 
   // Cache file 'stat' results and directories with absolutely paths.
   if (!Status.isDirectory() || llvm::sys::path::is_absolute(Path))
     StatCalls[Path] = Status;
 
-  return CacheExists;
+  return std::error_code();
 }