[clang] [clang] NFCI: Make `ModuleFile::File` non-optional (PR #74892)

Jan Svoboda via cfe-commits cfe-commits at lists.llvm.org
Fri Dec 8 14:41:59 PST 2023


https://github.com/jansvoboda11 created https://github.com/llvm/llvm-project/pull/74892

AFAICT, `ModuleFile::File` can be `std::nullopt` only for PCM files loaded from the standard input. This patch starts setting that variable to `FileManager::getSTDIN()` in that case, which makes it possible to remove the optionality, and also simplifies code that actually reads the file.

This is part of an effort to get rid of `Optional{File,Directory}EntryRefDegradesTo{File,Directory}EntryPtr`.

>From fb5c8c9fe856aaa2a314effa26486d5fbf019140 Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Thu, 7 Dec 2023 14:04:35 -0800
Subject: [PATCH] [clang] NFCI: Make `ModuleFile::File` non-optional

---
 .../include/clang/Serialization/ModuleFile.h  |  6 +--
 clang/lib/Serialization/ASTReader.cpp         |  2 +-
 clang/lib/Serialization/ASTWriter.cpp         |  2 +-
 clang/lib/Serialization/GlobalModuleIndex.cpp |  4 +-
 clang/lib/Serialization/ModuleManager.cpp     | 54 ++++++++-----------
 5 files changed, 29 insertions(+), 39 deletions(-)

diff --git a/clang/include/clang/Serialization/ModuleFile.h b/clang/include/clang/Serialization/ModuleFile.h
index 48be8676cc26a4..70ab61dec8b6bf 100644
--- a/clang/include/clang/Serialization/ModuleFile.h
+++ b/clang/include/clang/Serialization/ModuleFile.h
@@ -123,8 +123,8 @@ class InputFile {
 /// other modules.
 class ModuleFile {
 public:
-  ModuleFile(ModuleKind Kind, unsigned Generation)
-      : Kind(Kind), Generation(Generation) {}
+  ModuleFile(ModuleKind Kind, FileEntryRef File, unsigned Generation)
+      : Kind(Kind), File(File), Generation(Generation) {}
   ~ModuleFile();
 
   // === General information ===
@@ -176,7 +176,7 @@ class ModuleFile {
   bool DidReadTopLevelSubmodule = false;
 
   /// The file entry for the module file.
-  OptionalFileEntryRefDegradesToFileEntryPtr File;
+  FileEntryRef File;
 
   /// The signature of the module file, which may be used instead of the size
   /// and modification time to identify this particular file.
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index f22da838424b41..49f25d6648c801 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -5786,7 +5786,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
                 PartialDiagnostic(diag::err_module_file_conflict,
                                   ContextObj->DiagAllocator)
                 << CurrentModule->getTopLevelModuleName() << CurFile->getName()
-                << F.File->getName();
+                << F.File.getName();
             return DiagnosticError::create(CurrentImportLoc, ConflictError);
           }
         }
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 6df815234e235f..38e8b8ccbe058d 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1413,7 +1413,7 @@ void ASTWriter::WriteControlBlock(Preprocessor &PP, ASTContext &Context,
 
       // If we have calculated signature, there is no need to store
       // the size or timestamp.
-      Record.push_back(M.Signature ? 0 : M.File->getSize());
+      Record.push_back(M.Signature ? 0 : M.File.getSize());
       Record.push_back(M.Signature ? 0 : getTimestampForOutput(M.File));
 
       llvm::append_range(Record, M.Signature);
diff --git a/clang/lib/Serialization/GlobalModuleIndex.cpp b/clang/lib/Serialization/GlobalModuleIndex.cpp
index fb80a1998d0efe..dd4fc3e009050f 100644
--- a/clang/lib/Serialization/GlobalModuleIndex.cpp
+++ b/clang/lib/Serialization/GlobalModuleIndex.cpp
@@ -342,8 +342,8 @@ bool GlobalModuleIndex::loadedModuleFile(ModuleFile *File) {
   //  If the size and modification time match what we expected, record this
   // module file.
   bool Failed = true;
-  if (File->File->getSize() == Info.Size &&
-      File->File->getModificationTime() == Info.ModTime) {
+  if (File->File.getSize() == Info.Size &&
+      File->File.getModificationTime() == Info.ModTime) {
     Info.File = File;
     ModulesByFile[File] = Known->second;
 
diff --git a/clang/lib/Serialization/ModuleManager.cpp b/clang/lib/Serialization/ModuleManager.cpp
index de4cd3d05853ac..7cf97125b8ef03 100644
--- a/clang/lib/Serialization/ModuleManager.cpp
+++ b/clang/lib/Serialization/ModuleManager.cpp
@@ -108,7 +108,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
 
   // Look for the file entry. This only fails if the expected size or
   // modification time differ.
-  OptionalFileEntryRefDegradesToFileEntryPtr Entry;
+  OptionalFileEntryRef Entry;
   if (Type == MK_ExplicitModule || Type == MK_PrebuiltModule) {
     // If we're not expecting to pull this file out of the module cache, it
     // might have a different mtime due to being moved across filesystems in
@@ -123,7 +123,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
     return OutOfDate;
   }
 
-  if (!Entry && FileName != "-") {
+  if (!Entry) {
     ErrorStr = "module file not found";
     return Missing;
   }
@@ -150,7 +150,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
   };
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  if (ModuleFile *ModuleEntry = Modules.lookup(*Entry)) {
     if (implicitModuleNamesMatch(Type, ModuleEntry, *Entry)) {
       // Check the stored signature.
       if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
@@ -163,10 +163,9 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
   }
 
   // Allocate a new module.
-  auto NewModule = std::make_unique<ModuleFile>(Type, Generation);
+  auto NewModule = std::make_unique<ModuleFile>(Type, *Entry, Generation);
   NewModule->Index = Chain.size();
   NewModule->FileName = FileName.str();
-  NewModule->File = Entry;
   NewModule->ImportLoc = ImportLoc;
   NewModule->InputFilesValidationTimestamp = 0;
 
@@ -198,21 +197,15 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
     Entry->closeFile();
     return OutOfDate;
   } else {
-    // Open the AST file.
-    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buf((std::error_code()));
-    if (FileName == "-") {
-      Buf = llvm::MemoryBuffer::getSTDIN();
-    } else {
-      // Get a buffer of the file and close the file descriptor when done.
-      // The file is volatile because in a parallel build we expect multiple
-      // compiler processes to use the same module file rebuilding it if needed.
-      //
-      // RequiresNullTerminator is false because module files don't need it, and
-      // this allows the file to still be mmapped.
-      Buf = FileMgr.getBufferForFile(*NewModule->File,
-                                     /*IsVolatile=*/true,
-                                     /*RequiresNullTerminator=*/false);
-    }
+    // Get a buffer of the file and close the file descriptor when done.
+    // The file is volatile because in a parallel build we expect multiple
+    // compiler processes to use the same module file rebuilding it if needed.
+    //
+    // RequiresNullTerminator is false because module files don't need it, and
+    // this allows the file to still be mmapped.
+    auto Buf = FileMgr.getBufferForFile(NewModule->File,
+                                        /*IsVolatile=*/true,
+                                        /*RequiresNullTerminator=*/false);
 
     if (!Buf) {
       ErrorStr = Buf.getError().message();
@@ -232,7 +225,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
     return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[*Entry] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -441,22 +434,19 @@ void ModuleManager::visit(llvm::function_ref<bool(ModuleFile &M)> Visitor,
 bool ModuleManager::lookupModuleFile(StringRef FileName, off_t ExpectedSize,
                                      time_t ExpectedModTime,
                                      OptionalFileEntryRef &File) {
-  File = std::nullopt;
-  if (FileName == "-")
+  if (FileName == "-") {
+    File = expectedToOptional(FileMgr.getSTDIN());
     return false;
+  }
 
   // Open the file immediately to ensure there is no race between stat'ing and
   // opening the file.
-  OptionalFileEntryRef FileOrErr =
-      expectedToOptional(FileMgr.getFileRef(FileName, /*OpenFile=*/true,
-                                            /*CacheFailure=*/false));
-  if (!FileOrErr)
-    return false;
-
-  File = *FileOrErr;
+  File = expectedToOptional(FileMgr.getFileRef(FileName, /*OpenFile=*/true,
+                                               /*CacheFailure=*/false));
 
-  if ((ExpectedSize && ExpectedSize != File->getSize()) ||
-      (ExpectedModTime && ExpectedModTime != File->getModificationTime()))
+  if (File &&
+      ((ExpectedSize && ExpectedSize != File->getSize()) ||
+       (ExpectedModTime && ExpectedModTime != File->getModificationTime())))
     // Do not destroy File, as it may be referenced. If we need to rebuild it,
     // it will be destroyed by removeModules.
     return true;



More information about the cfe-commits mailing list