dexonsmith updated this revision to Diff 298144.
dexonsmith added a comment.
Cleaned up the (fixed) `Invalid` logic to reduce the size of the diff and use a
consistent pattern. Thanks for you patience with the false start; I think this
is ready now.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89348/new/
https://reviews.llvm.org/D89348
Files:
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: clang/lib/Serialization/ASTWriter.cpp
===================================================================
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2002,9 +2002,9 @@
// 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 @@
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);
Index: clang/lib/Basic/SourceManager.cpp
===================================================================
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -118,18 +118,25 @@
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 @@
// 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 @@
<< 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 @@
// 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 @@
<< 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 @@
<< 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 @@
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 @@
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 @@
}
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 @@
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 @@
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 @@
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 @@
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: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8683,12 +8683,10 @@
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: clang/include/clang/Basic/SourceManager.h
===================================================================
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -184,13 +184,21 @@
///
/// \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.
///
@@ -962,6 +970,24 @@
///
/// If there is an error opening this buffer the first time, this
/// manufactures a temporary buffer and returns a non-empty error string.
+ llvm::Optional<llvm::MemoryBufferRef>
+ getBufferOrNone(FileID FID, SourceLocation Loc = SourceLocation()) {
+ 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 +999,11 @@
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 +1043,18 @@
/// \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: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -305,22 +305,8 @@
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,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
