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

[Jan Svoboda via cfe-commits]

Author: Jan Svoboda
Date: 2022-01-21T13:04:25+01:00
New Revision: ced077e1ba52ec2937aab538e9b6fa5149f8c567

URL: https://github.com/llvm/llvm-project/commit/ced077e1ba52ec2937aab538e9b6fa5149f8c567
DIFF: https://github.com/llvm/llvm-project/commit/ced077e1ba52ec2937aab538e9b6fa5149f8c567.diff

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

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()`.

Reviewed By: dexonsmith

Differential Revision: https://reviews.llvm.org/D115935

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 1358950b437c8..08a60fe780f50 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -57,25 +57,26 @@ class CachedFileSystemEntry {
     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();
@@ -94,21 +95,31 @@ class CachedFileSystemEntry {
     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.getError();
+  }
+
+  /// \returns The entry status.
+  llvm::vfs::Status getStatus() const {
     assert(isInitialized() && "not initialized");
-    return MaybeStat;
+    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;
   }
 
@@ -183,19 +194,25 @@ class EntryRef {
   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();
   }

diff  --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index acceec690c11e..6b8c692335bd6 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -164,7 +164,7 @@ DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
 
   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.
@@ -194,7 +194,7 @@ DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
 
   // Store the result in the local cache.
   Entry = &SharedCacheEntry.Value;
-  return EntryRef(ShouldBeMinimized, *Entry);
+  return EntryRef(ShouldBeMinimized, *Entry).unwrapError();
 }
 
 llvm::ErrorOr<llvm::vfs::Status>
@@ -241,16 +241,15 @@ class MinimizedVFSFile final : public llvm::vfs::File {
 
 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)