clang/Basic: Replace ContentCache::getBuffer with Optional semantics
authorDuncan P. N. Exon Smith <dexonsmith@apple.com>
Tue, 13 Oct 2020 22:09:47 +0000 (18:09 -0400)
committerDuncan P. N. Exon Smith <dexonsmith@apple.com>
Wed, 14 Oct 2020 19:55:18 +0000 (15:55 -0400)
Remove `ContentCache::getBuffer`, which always returned a
dereferenceable `MemoryBuffer*` and had a `bool*Invalid` out parameter,
and replace it with:

- `ContentCache::getBufferOrNone`, which returns
  `Optional<MemoryBufferRef>`. This is the new API that consumers should
  use. Later it could be renamed to `getBuffer`, but intentionally using
  a different name to root out any unexpected callers.
- `ContentCache::getBufferPointer`, which returns `MemoryBuffer*` with
  "optional" semantics. This is `private` to avoid growing callers and
  `SourceManager` has temporarily been made a `friend` to access it.
  Later paches will update the transitive callers to not need a raw
  pointer, and eventually this will be deleted.

No functionality change intended here.

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

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang/include/clang/Basic/SourceManager.h
clang/lib/AST/ASTImporter.cpp
clang/lib/Basic/SourceManager.cpp
clang/lib/Serialization/ASTWriter.cpp

index 079d574..2b137d0 100644 (file)
@@ -305,22 +305,8 @@ static bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line,
 
 static llvm::Optional<StringRef> getBuffer(const SourceManager &SM, FileID File,
                                            bool AllowIO) {
-  // This is similar to the implementation of SourceManager::getBufferData(),
-  // but uses ContentCache::getRawBuffer() rather than getBuffer() if
-  // AllowIO=false, to avoid triggering file I/O if the file contents aren't
-  // already mapped.
-  bool CharDataInvalid = false;
-  const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(File, &CharDataInvalid);
-  if (CharDataInvalid || !Entry.isFile())
-    return llvm::None;
-  const SrcMgr::ContentCache *Cache = Entry.getFile().getContentCache();
-  const llvm::MemoryBuffer *Buffer =
-      AllowIO ? Cache->getBuffer(SM.getDiagnostics(), SM.getFileManager(),
-                                 SourceLocation(), &CharDataInvalid)
-              : Cache->getRawBuffer();
-  if (!Buffer || CharDataInvalid)
-    return llvm::None;
-  return Buffer->getBuffer();
+  return AllowIO ? SM.getBufferDataOrNone(File)
+                 : SM.getBufferDataIfLoaded(File);
 }
 
 static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
index 1e6a800..e34fe45 100644 (file)
@@ -184,13 +184,21 @@ namespace SrcMgr {
     ///
     /// \param Loc If specified, is the location that invalid file diagnostics
     ///   will be emitted at.
+    llvm::Optional<llvm::MemoryBufferRef>
+    getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
+                    SourceLocation Loc = SourceLocation()) const;
+
+  private:
+    /// Returns pointer to memory buffer.
     ///
-    /// \param Invalid If non-NULL, will be set \c true if an error occurred.
-    const llvm::MemoryBuffer *getBuffer(DiagnosticsEngine &Diag,
-                                        FileManager &FM,
-                                        SourceLocation Loc = SourceLocation(),
-                                        bool *Invalid = nullptr) const;
+    /// TODO: SourceManager needs access to this for now, but once that's done
+    /// we should remove this API.
+    const llvm::MemoryBuffer *getBufferPointer(DiagnosticsEngine &Diag,
+                                               FileManager &FM,
+                                               SourceLocation Loc) const;
+    friend class clang::SourceManager;
 
+  public:
     /// Returns the size of the content encapsulated by this
     /// ContentCache.
     ///
@@ -960,8 +968,25 @@ public:
 
   /// Return the buffer for the specified FileID.
   ///
+  /// If there is an error opening this buffer the first time, return None.
+  llvm::Optional<llvm::MemoryBufferRef>
+  getBufferOrNone(FileID FID, SourceLocation Loc = SourceLocation()) const {
+    bool MyInvalid = false;
+    const SrcMgr::SLocEntry &Entry = getSLocEntry(FID, &MyInvalid);
+    if (MyInvalid || !Entry.isFile())
+      return None;
+
+    return Entry.getFile().getContentCache()->getBufferOrNone(
+        Diag, getFileManager(), Loc);
+  }
+
+  /// Return the buffer for the specified FileID.
+  ///
   /// If there is an error opening this buffer the first time, this
   /// manufactures a temporary buffer and returns a non-empty error string.
+  ///
+  /// TODO: Update users of Invalid to call getBufferOrNone and change return
+  /// type to MemoryBufferRef.
   const llvm::MemoryBuffer *getBuffer(FileID FID, SourceLocation Loc,
                                       bool *Invalid = nullptr) const {
     bool MyInvalid = false;
@@ -973,8 +998,11 @@ public:
       return getFakeBufferForRecovery();
     }
 
-    return Entry.getFile().getContentCache()->getBuffer(Diag, getFileManager(),
-                                                        Loc, Invalid);
+    auto *B = Entry.getFile().getContentCache()->getBufferPointer(
+        Diag, getFileManager(), Loc);
+    if (Invalid)
+      *Invalid = !B;
+    return B ? B : getFakeBufferForRecovery();
   }
 
   const llvm::MemoryBuffer *getBuffer(FileID FID,
@@ -1014,6 +1042,18 @@ public:
   /// \param Invalid If non-NULL, will be set true if an error occurred.
   StringRef getBufferData(FileID FID, bool *Invalid = nullptr) const;
 
+  /// Return a StringRef to the source buffer data for the
+  /// specified FileID, returning None if invalid.
+  ///
+  /// \param FID The file ID whose contents will be returned.
+  llvm::Optional<StringRef> getBufferDataOrNone(FileID FID) const;
+
+  /// Return a StringRef to the source buffer data for the
+  /// specified FileID, returning None if it's not yet loaded.
+  ///
+  /// \param FID The file ID whose contents will be returned.
+  llvm::Optional<StringRef> getBufferDataIfLoaded(FileID FID) const;
+
   /// Get the number of FileIDs (files and macros) that were created
   /// during preprocessing of \p FID, including it.
   unsigned getNumCreatedFIDsForFileID(FileID FID) const {
index 29fcf6c..a5c3a5e 100644 (file)
@@ -8696,12 +8696,10 @@ Expected<FileID> ASTImporter::Import(FileID FromID, bool IsBuiltin) {
 
     if (ToID.isInvalid() || IsBuiltin) {
       // FIXME: We want to re-use the existing MemoryBuffer!
-      bool Invalid = true;
-      const llvm::MemoryBuffer *FromBuf =
-          Cache->getBuffer(FromContext.getDiagnostics(),
-                           FromSM.getFileManager(), SourceLocation{}, &Invalid);
-      if (!FromBuf || Invalid)
-        // FIXME: Use a new error kind?
+      llvm::Optional<llvm::MemoryBufferRef> FromBuf =
+          Cache->getBufferOrNone(FromContext.getDiagnostics(),
+                                 FromSM.getFileManager(), SourceLocation{});
+      if (!FromBuf)
         return llvm::make_error<ImportError>(ImportError::Unknown);
 
       std::unique_ptr<llvm::MemoryBuffer> ToBuf =
index 0f19440..1e19151 100644 (file)
@@ -118,18 +118,25 @@ const char *ContentCache::getInvalidBOM(StringRef BufStr) {
   return InvalidBOM;
 }
 
-const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
-                                                  FileManager &FM,
-                                                  SourceLocation Loc,
-                                                  bool *Invalid) const {
+llvm::Optional<llvm::MemoryBufferRef>
+ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
+                              SourceLocation Loc) const {
+  if (auto *B = getBufferPointer(Diag, FM, Loc))
+    return B->getMemBufferRef();
+  return None;
+}
+
+const llvm::MemoryBuffer *
+ContentCache::getBufferPointer(DiagnosticsEngine &Diag, FileManager &FM,
+                               SourceLocation Loc) const {
   // Lazily create the Buffer for ContentCaches that wrap files.  If we already
   // computed it, just return what we have.
-  if (Buffer.getPointer() || !ContentsEntry) {
-    if (Invalid)
-      *Invalid = isBufferInvalid();
-
-    return Buffer.getPointer();
-  }
+  if (isBufferInvalid())
+    return nullptr;
+  if (auto *B = Buffer.getPointer())
+    return B;
+  if (!ContentsEntry)
+    return nullptr;
 
   // Check that the file's size fits in an 'unsigned' (with room for a
   // past-the-end value). This is deeply regrettable, but various parts of
@@ -138,13 +145,6 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
   // miserably on large source files.
   if ((uint64_t)ContentsEntry->getSize() >=
       std::numeric_limits<unsigned>::max()) {
-    // We can't make a memory buffer of the required size, so just make a small
-    // one. We should never hit a situation where we've already parsed to a
-    // later offset of the file, so it shouldn't matter that the buffer is
-    // smaller than the file.
-    Buffer.setPointer(
-        llvm::MemoryBuffer::getMemBuffer("", ContentsEntry->getName())
-            .release());
     if (Diag.isDiagnosticInFlight())
       Diag.SetDelayedDiagnostic(diag::err_file_too_large,
                                 ContentsEntry->getName());
@@ -153,8 +153,7 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
         << ContentsEntry->getName();
 
     Buffer.setInt(Buffer.getInt() | InvalidFlag);
-    if (Invalid) *Invalid = true;
-    return Buffer.getPointer();
+    return nullptr;
   }
 
   auto BufferOrError = FM.getBufferForFile(ContentsEntry, IsFileVolatile);
@@ -164,20 +163,7 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
   // exists. Most likely, we were using a stat cache with an invalid entry but
   // the file could also have been removed during processing. Since we can't
   // really deal with this situation, just create an empty buffer.
-  //
-  // FIXME: This is definitely not ideal, but our immediate clients can't
-  // currently handle returning a null entry here. Ideally we should detect
-  // that we are in an inconsistent situation and error out as quickly as
-  // possible.
   if (!BufferOrError) {
-    StringRef FillStr("<<<MISSING SOURCE FILE>>>\n");
-    auto BackupBuffer = llvm::WritableMemoryBuffer::getNewUninitMemBuffer(
-        ContentsEntry->getSize(), "<invalid>");
-    char *Ptr = BackupBuffer->getBufferStart();
-    for (unsigned i = 0, e = ContentsEntry->getSize(); i != e; ++i)
-      Ptr[i] = FillStr[i % FillStr.size()];
-    Buffer.setPointer(BackupBuffer.release());
-
     if (Diag.isDiagnosticInFlight())
       Diag.SetDelayedDiagnostic(diag::err_cannot_open_file,
                                 ContentsEntry->getName(),
@@ -187,9 +173,7 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
           << ContentsEntry->getName() << BufferOrError.getError().message();
 
     Buffer.setInt(Buffer.getInt() | InvalidFlag);
-
-    if (Invalid) *Invalid = true;
-    return Buffer.getPointer();
+    return nullptr;
   }
 
   Buffer.setPointer(BufferOrError->release());
@@ -205,8 +189,7 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
         << ContentsEntry->getName();
 
     Buffer.setInt(Buffer.getInt() | InvalidFlag);
-    if (Invalid) *Invalid = true;
-    return Buffer.getPointer();
+    return nullptr;
   }
 
   // If the buffer is valid, check to see if it has a UTF Byte Order Mark
@@ -219,11 +202,9 @@ const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
     Diag.Report(Loc, diag::err_unsupported_bom)
       << InvalidBOM << ContentsEntry->getName();
     Buffer.setInt(Buffer.getInt() | InvalidFlag);
+    return nullptr;
   }
 
-  if (Invalid)
-    *Invalid = isBufferInvalid();
-
   return Buffer.getPointer();
 }
 
@@ -727,7 +708,10 @@ const llvm::MemoryBuffer *
 SourceManager::getMemoryBufferForFile(const FileEntry *File, bool *Invalid) {
   const SrcMgr::ContentCache *IR = getOrCreateContentCache(File);
   assert(IR && "getOrCreateContentCache() cannot return NULL");
-  return IR->getBuffer(Diag, getFileManager(), SourceLocation(), Invalid);
+  auto *B = IR->getBufferPointer(Diag, getFileManager(), SourceLocation());
+  if (Invalid)
+    *Invalid = !B;
+  return B ? B : getFakeBufferForRecovery();
 }
 
 void SourceManager::overrideFileContents(const FileEntry *SourceFile,
@@ -786,23 +770,35 @@ Optional<FileEntryRef> SourceManager::getFileEntryRefForID(FileID FID) const {
 }
 
 StringRef SourceManager::getBufferData(FileID FID, bool *Invalid) const {
+  auto B = getBufferDataOrNone(FID);
+  if (Invalid)
+    *Invalid = !B;
+  return B ? *B : "<<<<<INVALID SOURCE LOCATION>>>>>";
+}
+
+llvm::Optional<StringRef>
+SourceManager::getBufferDataIfLoaded(FileID FID) const {
   bool MyInvalid = false;
   const SLocEntry &SLoc = getSLocEntry(FID, &MyInvalid);
-  if (!SLoc.isFile() || MyInvalid) {
-    if (Invalid)
-      *Invalid = true;
-    return "<<<<<INVALID SOURCE LOCATION>>>>>";
-  }
+  if (!SLoc.isFile() || MyInvalid)
+    return None;
 
-  const llvm::MemoryBuffer *Buf = SLoc.getFile().getContentCache()->getBuffer(
-      Diag, getFileManager(), SourceLocation(), &MyInvalid);
-  if (Invalid)
-    *Invalid = MyInvalid;
+  if (const llvm::MemoryBuffer *Buf =
+          SLoc.getFile().getContentCache()->getRawBuffer())
+    return Buf->getBuffer();
+  return None;
+}
 
-  if (MyInvalid)
-    return "<<<<<INVALID SOURCE LOCATION>>>>>";
+llvm::Optional<StringRef> SourceManager::getBufferDataOrNone(FileID FID) const {
+  bool MyInvalid = false;
+  const SLocEntry &SLoc = getSLocEntry(FID, &MyInvalid);
+  if (!SLoc.isFile() || MyInvalid)
+    return None;
 
-  return Buf->getBuffer();
+  if (auto B = SLoc.getFile().getContentCache()->getBufferOrNone(
+          Diag, getFileManager(), SourceLocation()))
+    return B->getBuffer();
+  return None;
 }
 
 //===----------------------------------------------------------------------===//
@@ -1219,12 +1215,13 @@ const char *SourceManager::getCharacterData(SourceLocation SL,
 
     return "<<<<INVALID BUFFER>>>>";
   }
-  const llvm::MemoryBuffer *Buffer =
-      Entry.getFile().getContentCache()->getBuffer(
-          Diag, getFileManager(), SourceLocation(), &CharDataInvalid);
+  llvm::Optional<llvm::MemoryBufferRef> Buffer =
+      Entry.getFile().getContentCache()->getBufferOrNone(Diag, getFileManager(),
+                                                         SourceLocation());
   if (Invalid)
-    *Invalid = CharDataInvalid;
-  return Buffer->getBufferStart() + (CharDataInvalid? 0 : LocInfo.second);
+    *Invalid = !Buffer;
+  return Buffer ? Buffer->getBufferStart() + LocInfo.second
+                : "<<<<INVALID BUFFER>>>>";
 }
 
 /// getColumnNumber - Return the column # for the specified file position.
@@ -1317,8 +1314,9 @@ static void ComputeLineNumbers(DiagnosticsEngine &Diag, ContentCache *FI,
                                llvm::BumpPtrAllocator &Alloc,
                                const SourceManager &SM, bool &Invalid) {
   // Note that calling 'getBuffer()' may lazily page in the file.
-  const MemoryBuffer *Buffer =
-      FI->getBuffer(Diag, SM.getFileManager(), SourceLocation(), &Invalid);
+  llvm::Optional<llvm::MemoryBufferRef> Buffer =
+      FI->getBufferOrNone(Diag, SM.getFileManager(), SourceLocation());
+  Invalid = !Buffer;
   if (Invalid)
     return;
 
@@ -1543,8 +1541,8 @@ PresumedLoc SourceManager::getPresumedLoc(SourceLocation Loc,
   StringRef Filename;
   if (C->OrigEntry)
     Filename = C->OrigEntry->getName();
-  else
-    Filename = C->getBuffer(Diag, getFileManager())->getBufferIdentifier();
+  else if (auto Buffer = C->getBufferOrNone(Diag, getFileManager()))
+    Filename = Buffer->getBufferIdentifier();
 
   unsigned LineNo = getLineNumber(LocInfo.first, LocInfo.second, &Invalid);
   if (Invalid)
@@ -1739,14 +1737,18 @@ SourceLocation SourceManager::translateLineCol(FileID FID,
       return SourceLocation();
   }
 
+  llvm::Optional<llvm::MemoryBufferRef> Buffer =
+      Content->getBufferOrNone(Diag, getFileManager());
+  if (!Buffer)
+    return SourceLocation();
+
   if (Line > Content->NumLines) {
-    unsigned Size = Content->getBuffer(Diag, getFileManager())->getBufferSize();
+    unsigned Size = Buffer->getBufferSize();
     if (Size > 0)
       --Size;
     return FileLoc.getLocWithOffset(Size);
   }
 
-  const llvm::MemoryBuffer *Buffer = Content->getBuffer(Diag, getFileManager());
   unsigned FilePos = Content->SourceLineCache[Line - 1];
   const char *Buf = Buffer->getBufferStart() + FilePos;
   unsigned BufLength = Buffer->getBufferSize() - FilePos;
index ea0e182..e793e61 100644 (file)
@@ -2002,9 +2002,9 @@ void ASTWriter::WriteSourceManagerBlock(SourceManager &SourceMgr,
         // We add one to the size so that we capture the trailing NULL
         // that is required by llvm::MemoryBuffer::getMemBuffer (on
         // the reader side).
-        const llvm::MemoryBuffer *Buffer =
-            Content->getBuffer(PP.getDiagnostics(), PP.getFileManager());
-        StringRef Name = Buffer->getBufferIdentifier();
+        llvm::Optional<llvm::MemoryBufferRef> Buffer =
+            Content->getBufferOrNone(PP.getDiagnostics(), PP.getFileManager());
+        StringRef Name = Buffer ? Buffer->getBufferIdentifier() : "";
         Stream.EmitRecordWithBlob(SLocBufferAbbrv, Record,
                                   StringRef(Name.data(), Name.size() + 1));
         EmitBlob = true;
@@ -2016,8 +2016,10 @@ void ASTWriter::WriteSourceManagerBlock(SourceManager &SourceMgr,
       if (EmitBlob) {
         // Include the implicit terminating null character in the on-disk buffer
         // if we're writing it uncompressed.
-        const llvm::MemoryBuffer *Buffer =
-            Content->getBuffer(PP.getDiagnostics(), PP.getFileManager());
+        llvm::Optional<llvm::MemoryBufferRef> Buffer =
+            Content->getBufferOrNone(PP.getDiagnostics(), PP.getFileManager());
+        if (!Buffer)
+          Buffer = llvm::MemoryBufferRef("<<<INVALID BUFFER>>>", "");
         StringRef Blob(Buffer->getBufferStart(), Buffer->getBufferSize() + 1);
         emitBlob(Stream, Blob, SLocBufferBlobCompressedAbbrv,
                  SLocBufferBlobAbbrv);