Use capacity to express optionality, check lifetime even in static functions, prevent using rvalue-ref

Author: jansvoboda11

clang/include/clang/Serialization/ASTReader.h

@@ -1342,50 +1342,48 @@ class ASTReader
   serialization::InputFile getInputFile(ModuleFile &F, unsigned ID,
                                         bool Complain = true);
 
-  /// Buffer we use as temporary storage backing resolved paths.
-  std::optional<SmallString<0>> PathBuf;
+  /// The buffer used as the temporary backing storage for resolved paths.
+  SmallString<0> PathBuf;
 
-  /// A RAII wrapper around \c StringRef that temporarily takes ownership of the
-  /// underlying buffer and gives it back on destruction.
+  /// A wrapper around StringRef that temporarily borrows the underlying buffer.
   class TemporarilyOwnedStringRef {
     StringRef String;
-    llvm::SaveAndRestore<std::optional<SmallString<0>>> TemporaryLoan;
+    llvm::SaveAndRestore<SmallString<0>> UnderlyingBuffer;
 
   public:
-    TemporarilyOwnedStringRef(StringRef S, std::optional<SmallString<0>> &Buf)
-        : String(S), TemporaryLoan(Buf, {}) {}
+    TemporarilyOwnedStringRef(StringRef S, SmallString<0> &UnderlyingBuffer)
+        : String(S), UnderlyingBuffer(UnderlyingBuffer, {}) {}
 
-    /// Returns the wrapped \c StringRef that must be outlived by \c this.
-    const StringRef *operator->() const { return &String; }
-    /// Returns the wrapped \c StringRef that must be outlived by \c this.
-    const StringRef &operator*() const { return String; }
+    /// Return the wrapped \c StringRef that must be outlived by \c this.
+    const StringRef *operator->() const & { return &String; }
+    const StringRef &operator*() const & { return String; }
+
+    /// Make it harder to get a \c StringRef that outlives \c this.
+    const StringRef *operator->() && = delete;
+    const StringRef &operator*() && = delete;
   };
 
 public:
-  /// Resolve \c Path in the context of module file \c M. The return value must
-  /// be destroyed before another call to \c ResolveImportPath.
-  TemporarilyOwnedStringRef ResolveImportedPath(StringRef Path, ModuleFile &M) {
-    return ResolveImportedPath(Path, M.BaseDirectory);
-  }
-  /// Resolve \c Path in the context of the \c Prefix directory. The return
-  /// value must be destroyed before another call to \c ResolveImportPath.
-  TemporarilyOwnedStringRef ResolveImportedPath(StringRef Path,
-                                                StringRef Prefix) {
-    assert(PathBuf && "Multiple overlapping calls to ResolveImportedPath");
-    StringRef ResolvedPath = ResolveImportedPath(*PathBuf, Path, Prefix);
-    return {ResolvedPath, PathBuf};
-  }
+  /// Get the buffer for resolving paths.
+  SmallString<0> &getPathBuf() { return PathBuf; }
 
-  /// Resolve \c Path in the context of module file \c M. The \c Buffer must
-  /// outlive the returned \c StringRef.
-  static StringRef ResolveImportedPath(SmallVectorImpl<char> &Buffer,
-                                       StringRef Path, ModuleFile &M) {
-    return ResolveImportedPath(Buffer, Path, M.BaseDirectory);
-  }
-  /// Resolve \c Path in the context of the \c Prefix directory. The \c Buffer
-  /// must outlive the returned \c StringRef.
-  static StringRef ResolveImportedPath(SmallVectorImpl<char> &Buffer,
-                                       StringRef Path, StringRef Prefix);
+  /// Resolve \c Path in the context of module file \c M. The return value
+  /// must go out of scope before the next call to \c ResolveImportedPath.
+  static TemporarilyOwnedStringRef
+  ResolveImportedPath(SmallString<0> &Buf, StringRef Path, ModuleFile &ModF);
+  /// Resolve \c Path in the context of the \c Prefix directory. The return
+  /// value must go out of scope before the next call to \c ResolveImportedPath.
+  static TemporarilyOwnedStringRef
+  ResolveImportedPath(SmallString<0> &Buf, StringRef Path, StringRef Prefix);
+
+  /// Resolve \c Path in the context of module file \c M.
+  static std::string ResolveImportedPathAndAllocate(SmallString<0> &Buf,
+                                                    StringRef Path,
+                                                    ModuleFile &ModF);
+  /// Resolve \c Path in the context of the \c Prefix directory.
+  static std::string ResolveImportedPathAndAllocate(SmallString<0> &Buf,
+                                                    StringRef Path,
+                                                    StringRef Prefix);
 
   /// Returns the first key declaration for the given declaration. This
   /// is one that is formerly-canonical (or still canonical) and whose module

clang/lib/Serialization/ASTReader.cpp

@@ -2048,7 +2048,8 @@ HeaderFileInfoTrait::getFile(const internal_key_type &Key) {
   if (!Key.Imported)
     return FileMgr.getOptionalFileRef(Key.Filename);
 
-  auto Resolved = Reader.ResolveImportedPath(Key.Filename, M);
+  auto Resolved =
+      ASTReader::ResolveImportedPath(Reader.getPathBuf(), Key.Filename, M);
   return FileMgr.getOptionalFileRef(*Resolved);
 }
 
@@ -2512,10 +2513,12 @@ InputFileInfo ASTReader::getInputFileInfo(ModuleFile &F, unsigned ID) {
   std::tie(R.FilenameAsRequested, R.Filename) = [&]() {
     uint16_t AsRequestedLength = Record[7];
 
+    StringRef NameAsRequestedRef = Blob.substr(0, AsRequestedLength);
+    StringRef NameRef = Blob.substr(AsRequestedLength);
+
     std::string NameAsRequested =
-        ResolveImportedPath(Blob.substr(0, AsRequestedLength), F)->str();
-    std::string Name =
-        ResolveImportedPath(Blob.substr(AsRequestedLength), F)->str();
+        ResolveImportedPathAndAllocate(PathBuf, NameAsRequestedRef, F);
+    std::string Name = ResolveImportedPathAndAllocate(PathBuf, NameRef, F);
 
     if (Name.empty())
       Name = NameAsRequested;
@@ -2741,18 +2744,38 @@ InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) {
   return IF;
 }
 
-/// If we are loading a relocatable PCH or module file, and the filename
-/// is not an absolute path, add the system or module root to the beginning of
-/// the file name.
-StringRef ASTReader::ResolveImportedPath(SmallVectorImpl<char> &Buffer,
-                                         StringRef Path, StringRef Prefix) {
+ASTReader::TemporarilyOwnedStringRef
+ASTReader::ResolveImportedPath(SmallString<0> &Buf, StringRef Path,
+                               ModuleFile &ModF) {
+  return ResolveImportedPath(Buf, Path, ModF.BaseDirectory);
+}
+
+ASTReader::TemporarilyOwnedStringRef
+ASTReader::ResolveImportedPath(SmallString<0> &Buf, StringRef Path,
+                               StringRef Prefix) {
+  assert(Buf.capacity() != 0 && "Overlapping ResolveImportedPath calls");
+
   if (Prefix.empty() || Path.empty() || llvm::sys::path::is_absolute(Path) ||
       Path == "<built-in>" || Path == "<command line>")
-    return Path;
+    return {Path, Buf};
 
-  Buffer.clear();
-  llvm::sys::path::append(Buffer, Prefix, Path);
-  return {Buffer.data(), Buffer.size()};
+  Buf.clear();
+  llvm::sys::path::append(Buf, Prefix, Path);
+  StringRef ResolvedPath{Buf.data(), Buf.size()};
+  return {ResolvedPath, Buf};
+}
+
+std::string ASTReader::ResolveImportedPathAndAllocate(SmallString<0> &Buf,
+                                                      StringRef P,
+                                                      ModuleFile &ModF) {
+  return ResolveImportedPathAndAllocate(Buf, P, ModF.BaseDirectory);
+}
+
+std::string ASTReader::ResolveImportedPathAndAllocate(SmallString<0> &Buf,
+                                                      StringRef P,
+                                                      StringRef Prefix) {
+  auto ResolvedPath = ResolveImportedPath(Buf, P, Prefix);
+  return ResolvedPath->str();
 }
 
 static bool isDiagnosedResult(ASTReader::ASTReadResult ARR, unsigned Caps) {
@@ -3180,8 +3203,8 @@ ASTReader::ReadControlBlock(ModuleFile &F,
     case ORIGINAL_FILE:
       F.OriginalSourceFileID = FileID::get(Record[0]);
       F.ActualOriginalSourceFileName = std::string(Blob);
-      F.OriginalSourceFileName =
-          ResolveImportedPath(F.ActualOriginalSourceFileName, F)->str();
+      F.OriginalSourceFileName = ResolveImportedPathAndAllocate(
+          PathBuf, F.ActualOriginalSourceFileName, F);
       break;
 
     case ORIGINAL_FILE_ID:
@@ -5470,7 +5493,8 @@ bool ASTReader::readASTFileControlBlock(
   RecordData Record;
   std::string ModuleDir;
   bool DoneWithControlBlock = false;
-  SmallString<256> PathBuf;
+  SmallString<0> PathBuf;
+  PathBuf.reserve(256);
   while (!DoneWithControlBlock) {
     Expected<llvm::BitstreamEntry> MaybeEntry = Stream.advance();
     if (!MaybeEntry) {
@@ -5554,8 +5578,8 @@ bool ASTReader::readASTFileControlBlock(
     case MODULE_MAP_FILE: {
       unsigned Idx = 0;
       std::string PathStr = ReadString(Record, Idx);
-      StringRef Path = ResolveImportedPath(PathBuf, PathStr, ModuleDir);
-      Listener.ReadModuleMapFile(Path);
+      auto Path = ResolveImportedPath(PathBuf, PathStr, ModuleDir);
+      Listener.ReadModuleMapFile(*Path);
       break;
     }
     case INPUT_FILE_OFFSETS: {
@@ -5602,9 +5626,9 @@ bool ASTReader::readASTFileControlBlock(
           break;
         case INPUT_FILE:
           bool Overridden = static_cast<bool>(Record[3]);
-          StringRef Filename = ResolveImportedPath(PathBuf, Blob, ModuleDir);
+          auto Filename = ResolveImportedPath(PathBuf, Blob, ModuleDir);
           shouldContinue = Listener.visitInputFile(
-              Filename, isSystemFile, Overridden, /*IsExplicitModule*/false);
+              *Filename, isSystemFile, Overridden, /*IsExplicitModule*/false);
           break;
         }
         if (!shouldContinue)
@@ -5640,9 +5664,8 @@ bool ASTReader::readASTFileControlBlock(
         Idx += 1 + 1 + ASTFileSignature::size;
         std::string ModuleName = ReadString(Record, Idx);
         std::string FilenameStr = ReadString(Record, Idx);
-        StringRef Filename =
-            ResolveImportedPath(PathBuf, FilenameStr, ModuleDir);
-        Listener.visitImport(ModuleName, Filename);
+        auto Filename = ResolveImportedPath(PathBuf, FilenameStr, ModuleDir);
+        Listener.visitImport(ModuleName, *Filename);
       }
       break;
     }
@@ -5895,7 +5918,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
       // FIXME: This doesn't work for framework modules as `Filename` is the
       //        name as written in the module file and does not include
       //        `Headers/`, so this path will never exist.
-      auto Filename = ResolveImportedPath(Blob, F);
+      auto Filename = ResolveImportedPath(PathBuf, Blob, F);
       if (auto Umbrella = PP.getFileManager().getOptionalFileRef(*Filename)) {
         if (!CurrentModule->getUmbrellaHeaderAsWritten()) {
           // FIXME: NameAsWritten
@@ -5924,14 +5947,14 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
       break;
 
     case SUBMODULE_TOPHEADER: {
-      auto HeaderName = ResolveImportedPath(Blob, F);
+      auto HeaderName = ResolveImportedPath(PathBuf, Blob, F);
       CurrentModule->addTopHeaderFilename(*HeaderName);
       break;
     }
 
     case SUBMODULE_UMBRELLA_DIR: {
       // See comments in SUBMODULE_UMBRELLA_HEADER
-      auto Dirname = ResolveImportedPath(Blob, F);
+      auto Dirname = ResolveImportedPath(PathBuf, Blob, F);
       if (auto Umbrella =
               PP.getFileManager().getOptionalDirectoryRef(*Dirname)) {
         if (!CurrentModule->getUmbrellaDirAsWritten()) {
@@ -9588,14 +9611,13 @@ std::string ASTReader::ReadString(const RecordDataImpl &Record, unsigned &Idx) {
 
 std::string ASTReader::ReadPath(ModuleFile &F, const RecordData &Record,
                                 unsigned &Idx) {
-  std::string Filename = ReadString(Record, Idx);
-  return ResolveImportedPath(Filename, F)->str();
+  return ReadPath(F.BaseDirectory, Record, Idx);
 }
 
 std::string ASTReader::ReadPath(StringRef BaseDirectory,
                                 const RecordData &Record, unsigned &Idx) {
   std::string Filename = ReadString(Record, Idx);
-  return ResolveImportedPath(Filename, BaseDirectory)->str();
+  return ResolveImportedPathAndAllocate(PathBuf, Filename, BaseDirectory);
 }
 
 VersionTuple ASTReader::ReadVersionTuple(const RecordData &Record,
@@ -10500,8 +10522,7 @@ ASTReader::ASTReader(Preprocessor &PP, InMemoryModuleCache &ModuleCache,
       UseGlobalIndex(UseGlobalIndex), CurrSwitchCaseStmts(&SwitchCaseStmts) {
   SourceMgr.setExternalSLocEntrySource(this);
 
-  PathBuf.emplace();
-  PathBuf->reserve(256);
+  PathBuf.reserve(256);
 
   for (const auto &Ext : Extensions) {
     auto BlockName = Ext->getExtensionMetadata().BlockName;