[clang][deps] NFC: Simplify handling of cached FS errors

[jansvoboda11]

Summary

The return types of some CachedFileSystemEntry member function are needlessly complex.

This patch attempts to simplify the code by unwrapping cached entries that represent errors early, and then asserting !isError().

[jansvoboda11]

Add more assertions.

[dexonsmith]

LGTM.

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

public:
   /// Minimize contents of the file.
   void minimizeFile();
 
   /// \returns True if the entry is initialized.
   bool isInitialized() const {
     return !MaybeStat || MaybeStat->isStatusKnown();
   }
 
+  /// \returns True if the entry is a filesystem error.
+  bool isError() const { return !MaybeStat; }
+
   /// \returns True if the current entry points to a directory.
-  bool isDirectory() const { return MaybeStat && MaybeStat->isDirectory(); }
+  bool isDirectory() const { return !isError() && MaybeStat->isDirectory(); }
 
-  /// \returns The error or the file's original contents.
-  llvm::ErrorOr<StringRef> getOriginalContents() const {
-    if (!MaybeStat)
-      return MaybeStat.getError();
-    assert(!MaybeStat->isDirectory() && "not a file");
+  /// \returns Original contents of the file.
+  StringRef getOriginalContents() const {
     assert(isInitialized() && "not initialized");
+    assert(!isError() && "error");
+    assert(!MaybeStat->isDirectory() && "not a file");
     assert(OriginalContents && "not read");
     return OriginalContents->getBuffer();
   }
 
-  /// \returns The error or the file's minimized contents.
-  llvm::ErrorOr<StringRef> getMinimizedContents() const {
-    if (!MaybeStat)
-      return MaybeStat.getError();
-    assert(!MaybeStat->isDirectory() && "not a file");
+  /// \returns Minimized contents of the file.
+  StringRef getMinimizedContents() const {
     assert(isInitialized() && "not initialized");
+    assert(!isError() && "error");
+    assert(!isDirectory() && "not a file");
     llvm::MemoryBuffer *Buffer = MinimizedContentsAccess.load();
     assert(Buffer && "not minimized");
     return Buffer->getBuffer();
   }
 
   /// \returns True if this entry represents a file that can be read.
   bool isReadable() const { return MaybeStat && !MaybeStat->isDirectory(); }
 
   /// \returns True if this cached entry needs to be updated.
   bool needsUpdate(bool ShouldBeMinimized) const {
     return isReadable() && needsMinimization(ShouldBeMinimized);
   }
 
   /// \returns True if the contents of this entry need to be minimized.
   bool needsMinimization(bool ShouldBeMinimized) const {
     return ShouldBeMinimized && !MinimizedContentsAccess.load();
   }
 
-  /// \returns The error or the status of the entry.
-  llvm::ErrorOr<llvm::vfs::Status> getStatus() const {
+  /// \returns The error.
+  std::error_code getError() const {
     assert(isInitialized() && "not initialized");
-    return MaybeStat;
+    return MaybeStat.getError();
+  }
+
+  /// \returns The entry status.
+  llvm::vfs::Status getStatus() const {
+    assert(isInitialized() && "not initialized");
+    assert(!isError() && "error");
+    return *MaybeStat;
   }
 
   /// \returns the name of the file.
   StringRef getName() const {
     assert(isInitialized() && "not initialized");
+    assert(!isError() && "error");
     return MaybeStat->getName();
   }
 
   /// Return the mapping between location -> distance that is used to speed up
   /// the block skipping in the preprocessor.
   const PreprocessorSkippedRangeMapping &getPPSkippedRangeMapping() const {
+    assert(!isError() && "error");
+    assert(!isDirectory() && "not a file");
     return PPSkippedRangeMapping;
   }
 
 private:
   llvm::ErrorOr<llvm::vfs::Status> MaybeStat;
   std::unique_ptr<llvm::MemoryBuffer> OriginalContents;
 
   /// Owning storage for the minimized file contents.
class EntryRef {
 
   /// The underlying cached entry.
   const CachedFileSystemEntry &Entry;
 
 public:
   EntryRef(bool Minimized, const CachedFileSystemEntry &Entry)
       : Minimized(Minimized), Entry(Entry) {}
 
-  llvm::ErrorOr<llvm::vfs::Status> getStatus() const {
-    auto MaybeStat = Entry.getStatus();
-    if (!MaybeStat || MaybeStat->isDirectory())
-      return MaybeStat;
-    return llvm::vfs::Status::copyWithNewSize(*MaybeStat,
-                                              getContents()->size());
+  llvm::vfs::Status getStatus() const {
+    llvm::vfs::Status Stat = Entry.getStatus();
+    if (Stat.isDirectory())
+      return Stat;
+    return llvm::vfs::Status::copyWithNewSize(Stat, getContents().size());
   }
 
+  bool isError() const { return Entry.isError(); }
   bool isDirectory() const { return Entry.isDirectory(); }
-
   StringRef getName() const { return Entry.getName(); }
 
-  llvm::ErrorOr<StringRef> getContents() const {
+  /// If the cached entry represents an error, promotes it into `ErrorOr`.
+  llvm::ErrorOr<EntryRef> unwrapError() const {
+    if (isError())
+      return Entry.getError();
+    return *this;
+  }
+
+  StringRef getContents() const {
     return Minimized ? Entry.getMinimizedContents()
                      : Entry.getOriginalContents();
   }
 
   const PreprocessorSkippedRangeMapping *getPPSkippedRangeMapping() const {
     return Minimized ? &Entry.getPPSkippedRangeMapping() : nullptr;
   }
 };

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

 
 llvm::ErrorOr<EntryRef>
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
     StringRef Filename) {
   bool ShouldBeMinimized = shouldMinimize(Filename);
 
   const auto *Entry = LocalCache.getCachedEntry(Filename);
   if (Entry && !Entry->needsUpdate(ShouldBeMinimized))
-    return EntryRef(ShouldBeMinimized, *Entry);
+    return EntryRef(ShouldBeMinimized, *Entry).unwrapError();
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
 
   auto &SharedCacheEntry = SharedCache.get(Filename);
   {
     std::lock_guard<std::mutex> LockGuard(SharedCacheEntry.ValueLock);
     CachedFileSystemEntry &CacheEntry = SharedCacheEntry.Value;
auto &SharedCacheEntry = SharedCache.get(Filename);
     // Only checking `needsMinimization` could lead to minimization of files
     // that we failed to load (such files don't have `OriginalContents`).
     if (CacheEntry.needsUpdate(ShouldBeMinimized))
       CacheEntry.minimizeFile();
   }
 
   // Store the result in the local cache.
   Entry = &SharedCacheEntry.Value;
-  return EntryRef(ShouldBeMinimized, *Entry);
+  return EntryRef(ShouldBeMinimized, *Entry).unwrapError();
 }
 
 llvm::ErrorOr<llvm::vfs::Status>
 DependencyScanningWorkerFilesystem::status(const Twine &Path) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
   llvm::ErrorOr<EntryRef> Result = getOrCreateFileSystemEntry(Filename);
private:
   std::unique_ptr<llvm::MemoryBuffer> Buffer;
   llvm::vfs::Status Stat;
 };
 
 } // end anonymous namespace
 
 llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> MinimizedVFSFile::create(
     EntryRef Entry, ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings) {
+  assert(!Entry.isError() && "error");
+
   if (Entry.isDirectory())
     return std::make_error_code(std::errc::is_a_directory);
 
-  llvm::ErrorOr<StringRef> Contents = Entry.getContents();
-  if (!Contents)
-    return Contents.getError();
   auto Result = std::make_unique<MinimizedVFSFile>(
-      llvm::MemoryBuffer::getMemBuffer(*Contents, Entry.getName(),
+      llvm::MemoryBuffer::getMemBuffer(Entry.getContents(), Entry.getName(),
                                        /*RequiresNullTerminator=*/false),
-      *Entry.getStatus());
+      Entry.getStatus());
 
   const auto *EntrySkipMappings = Entry.getPPSkippedRangeMapping();
   if (EntrySkipMappings && !EntrySkipMappings->empty() && PPSkipMappings)
     (*PPSkipMappings)[Result->Buffer->getBufferStart()] = EntrySkipMappings;
 
   return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(
       std::unique_ptr<llvm::vfs::File>(std::move(Result)));
 }