[PATCH] D67030: ContentCache: Simplify by always owning the MemoryBuffer

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG296314516d10: ContentCache: Simplify by always owning the 
MemoryBuffer (authored by dexonsmith).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D67030?vs=298257=299529#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67030/new/

https://reviews.llvm.org/D67030

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/CompilerInstance.cpp

Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -346,10 +346,16 @@
   continue;
 }
 
-// Override the contents of the "from" file with the contents of
-// the "to" file.
-SourceMgr.overrideFileContents(FromFile, RB.second,
-   InitOpts.RetainRemappedFileBuffers);
+// Override the contents of the "from" file with the contents of the
+// "to" file. If the caller owns the buffers, then pass a MemoryBufferRef;
+// otherwise, pass as a std::unique_ptr to transfer ownership
+// to the SourceManager.
+if (InitOpts.RetainRemappedFileBuffers)
+  SourceMgr.overrideFileContents(FromFile, RB.second->getMemBufferRef());
+else
+  SourceMgr.overrideFileContents(
+  FromFile, std::unique_ptr(
+const_cast(RB.second)));
   }
 
   // Remap files in the source manager (with other files).
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -49,28 +49,22 @@
 // SourceManager Helper Classes
 //===--===//
 
-ContentCache::~ContentCache() {
-  if (shouldFreeBuffer())
-delete Buffer.getPointer();
-}
-
 /// getSizeBytesMapped - Returns the number of bytes actually mapped for this
 /// ContentCache. This can be 0 if the MemBuffer was not actually expanded.
 unsigned ContentCache::getSizeBytesMapped() const {
-  return Buffer.getPointer() ? Buffer.getPointer()->getBufferSize() : 0;
+  return Buffer ? Buffer->getBufferSize() : 0;
 }
 
 /// Returns the kind of memory used to back the memory buffer for
 /// this content cache.  This is used for performance analysis.
 llvm::MemoryBuffer::BufferKind ContentCache::getMemoryBufferKind() const {
-  assert(Buffer.getPointer());
+  assert(Buffer);
 
   // Should be unreachable, but keep for sanity.
-  if (!Buffer.getPointer())
+  if (!Buffer)
 return llvm::MemoryBuffer::MemoryBuffer_Malloc;
 
-  const llvm::MemoryBuffer *buf = Buffer.getPointer();
-  return buf->getBufferKind();
+  return Buffer->getBufferKind();
 }
 
 /// getSize - Returns the size of the content encapsulated by this ContentCache.
@@ -78,21 +72,8 @@
 ///  scratch buffer.  If the ContentCache encapsulates a source file, that
 ///  file is not lazily brought in from disk to satisfy this query.
 unsigned ContentCache::getSize() const {
-  return Buffer.getPointer() ? (unsigned) Buffer.getPointer()->getBufferSize()
- : (unsigned) ContentsEntry->getSize();
-}
-
-void ContentCache::replaceBuffer(const llvm::MemoryBuffer *B, bool DoNotFree) {
-  if (B && B == Buffer.getPointer()) {
-assert(0 && "Replacing with the same buffer");
-Buffer.setInt(DoNotFree? DoNotFreeFlag : 0);
-return;
-  }
-
-  if (shouldFreeBuffer())
-delete Buffer.getPointer();
-  Buffer.setPointer(B);
-  Buffer.setInt((B && DoNotFree) ? DoNotFreeFlag : 0);
+  return Buffer ? (unsigned)Buffer->getBufferSize()
+: (unsigned)ContentsEntry->getSize();
 }
 
 const char *ContentCache::getInvalidBOM(StringRef BufStr) {
@@ -125,8 +106,8 @@
   // computed it, just return what we have.
   if (IsBufferInvalid)
 return None;
-  if (auto *B = Buffer.getPointer())
-return B->getMemBufferRef();
+  if (Buffer)
+return Buffer->getMemBufferRef();
   if (!ContentsEntry)
 return None;
 
@@ -168,7 +149,7 @@
 return None;
   }
 
-  Buffer.setPointer(BufferOrError->release());
+  Buffer = std::move(*BufferOrError);
 
   // Check that the file's size is the same as in the file entry (which may
   // have come from a stat cache).
@@ -187,7 +168,7 @@
   // If the buffer is valid, check to see if it has a UTF Byte Order Mark
   // (BOM).  We only support UTF-8 with and without a BOM right now.  See
   // http://en.wikipedia.org/wiki/Byte_order_mark for more information.
-  StringRef BufStr = Buffer.getPointer()->getBuffer();
+  StringRef BufStr = Buffer->getBuffer();
   const char *InvalidBOM = getInvalidBOM(BufStr);
 
   if (InvalidBOM) {
@@ -197,7 +178,7 @@
 return None;
   }
 
-  return Buffer.getPointer()->getMemBufferRef();
+  return Buffer->getMemBufferRef();
 }
 
 

[PATCH] D67030: ContentCache: Simplify by always owning the MemoryBuffer

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

ping

In D67030#2331280 , @dexonsmith wrote:

> Rebased on top of https://reviews.llvm.org/D89431 (the tests haven't finished 
> yet; I'll update if necessary, but this is pretty straightforward).

(FTR, the tests passed as expected.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67030/new/

https://reviews.llvm.org/D67030

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67030: ContentCache: Simplify by always owning the MemoryBuffer

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith updated this revision to Diff 298257.
dexonsmith edited the summary of this revision.
dexonsmith added a comment.

Rebased on top of https://reviews.llvm.org/D89431 (the tests haven't finished 
yet; I'll update if necessary, but this is pretty straightforward).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67030/new/

https://reviews.llvm.org/D67030

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp

Index: lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
===
--- lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
+++ lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
@@ -168,7 +168,7 @@
   clang::SourceManager SM(diags, file_mgr);
   auto buf = llvm::MemoryBuffer::getMemBuffer(full_source);
 
-  FileID FID = SM.createFileID(clang::SourceManager::Unowned, buf.get());
+  FileID FID = SM.createFileID(buf->getMemBufferRef());
 
   // Let's just enable the latest ObjC and C++ which should get most tokens
   // right.
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -221,7 +221,7 @@
   clang::SourceManager SM(diags, file_mgr);
   auto buf = llvm::MemoryBuffer::getMemBuffer(body);
 
-  FileID FID = SM.createFileID(clang::SourceManager::Unowned, buf.get());
+  FileID FID = SM.createFileID(buf->getMemBufferRef());
 
   // Let's just enable the latest ObjC and C++ which should get most tokens
   // right.
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -346,10 +346,16 @@
   continue;
 }
 
-// Override the contents of the "from" file with the contents of
-// the "to" file.
-SourceMgr.overrideFileContents(FromFile, RB.second,
-   InitOpts.RetainRemappedFileBuffers);
+// Override the contents of the "from" file with the contents of the
+// "to" file. If the caller owns the buffers, then pass a MemoryBufferRef;
+// otherwise, pass as a std::unique_ptr to transfer ownership
+// to the SourceManager.
+if (InitOpts.RetainRemappedFileBuffers)
+  SourceMgr.overrideFileContents(FromFile, RB.second->getMemBufferRef());
+else
+  SourceMgr.overrideFileContents(
+  FromFile, std::unique_ptr(
+const_cast(RB.second)));
   }
 
   // Remap files in the source manager (with other files).
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -49,28 +49,22 @@
 // SourceManager Helper Classes
 //===--===//
 
-ContentCache::~ContentCache() {
-  if (shouldFreeBuffer())
-delete Buffer.getPointer();
-}
-
 /// getSizeBytesMapped - Returns the number of bytes actually mapped for this
 /// ContentCache. This can be 0 if the MemBuffer was not actually expanded.
 unsigned ContentCache::getSizeBytesMapped() const {
-  return Buffer.getPointer() ? Buffer.getPointer()->getBufferSize() : 0;
+  return Buffer ? Buffer->getBufferSize() : 0;
 }
 
 /// Returns the kind of memory used to back the memory buffer for
 /// this content cache.  This is used for performance analysis.
 llvm::MemoryBuffer::BufferKind ContentCache::getMemoryBufferKind() const {
-  assert(Buffer.getPointer());
+  assert(Buffer);
 
   // Should be unreachable, but keep for sanity.
-  if (!Buffer.getPointer())
+  if (!Buffer)
 return llvm::MemoryBuffer::MemoryBuffer_Malloc;
 
-  const llvm::MemoryBuffer *buf = Buffer.getPointer();
-  return buf->getBufferKind();
+  return Buffer->getBufferKind();
 }
 
 /// getSize - Returns the size of the content encapsulated by this ContentCache.
@@ -78,21 +72,8 @@
 ///  scratch buffer.  If the ContentCache encapsulates a source file, that
 ///  file is not lazily brought in from disk to satisfy this query.
 unsigned ContentCache::getSize() const {
-  return Buffer.getPointer() ? (unsigned) Buffer.getPointer()->getBufferSize()
- : (unsigned) ContentsEntry->getSize();
-}
-
-void ContentCache::replaceBuffer(const llvm::MemoryBuffer *B, bool DoNotFree) {
-  if (B && B == Buffer.getPointer()) {
-assert(0 && "Replacing with the same buffer");
-Buffer.setInt(DoNotFree? DoNotFreeFlag : 0);
-return;
-  }
-
-  if (shouldFreeBuffer())
-delete 

[PATCH] D67030: ContentCache: Simplify by always owning the MemoryBuffer

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

ping: Besides rebasing, any concerns here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67030/new/

https://reviews.llvm.org/D67030

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67030: ContentCache: Simplify by always owning the MemoryBuffer

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.
dexonsmith added reviewers: arphaman, Bigcheese, rsmith.
Herald added a subscriber: ributzka.

This changes ContentCache::Buffer to use `std::unique_ptr`
instead of the PointerIntPair.  It drops the (mostly unused) `DoNotFree`
bit in favour of creating a new, non-owning MemoryBuffer instance.


https://reviews.llvm.org/D67030

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp

Index: lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
===
--- lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
+++ lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
@@ -151,7 +151,7 @@
   clang::SourceManager SM(diags, file_mgr);
   auto buf = llvm::MemoryBuffer::getMemBuffer(full_source);
 
-  FileID FID = SM.createFileID(clang::SourceManager::Unowned, buf.get());
+  FileID FID = SM.createFileID(buf->getMemBufferRef());
 
   // Let's just enable the latest ObjC and C++ which should get most tokens
   // right.
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -336,8 +336,12 @@
 
 // Override the contents of the "from" file with the contents of
 // the "to" file.
-SourceMgr.overrideFileContents(FromFile, RB.second,
-   InitOpts.RetainRemappedFileBuffers);
+if (InitOpts.RetainRemappedFileBuffers)
+  SourceMgr.overrideFileContents(FromFile, *RB.second);
+else
+  SourceMgr.overrideFileContents(
+  FromFile, std::unique_ptr(
+const_cast(RB.second)));
   }
 
   // Remap files in the source manager (with other files).
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -49,28 +49,22 @@
 // SourceManager Helper Classes
 //===--===//
 
-ContentCache::~ContentCache() {
-  if (shouldFreeBuffer())
-delete Buffer.getPointer();
-}
-
 /// getSizeBytesMapped - Returns the number of bytes actually mapped for this
 /// ContentCache. This can be 0 if the MemBuffer was not actually expanded.
 unsigned ContentCache::getSizeBytesMapped() const {
-  return Buffer.getPointer() ? Buffer.getPointer()->getBufferSize() : 0;
+  return Buffer ? Buffer->getBufferSize() : 0;
 }
 
 /// Returns the kind of memory used to back the memory buffer for
 /// this content cache.  This is used for performance analysis.
 llvm::MemoryBuffer::BufferKind ContentCache::getMemoryBufferKind() const {
-  assert(Buffer.getPointer());
+  assert(Buffer);
 
   // Should be unreachable, but keep for sanity.
-  if (!Buffer.getPointer())
+  if (!Buffer)
 return llvm::MemoryBuffer::MemoryBuffer_Malloc;
 
-  const llvm::MemoryBuffer *buf = Buffer.getPointer();
-  return buf->getBufferKind();
+  return Buffer->getBufferKind();
 }
 
 /// getSize - Returns the size of the content encapsulated by this ContentCache.
@@ -78,23 +72,8 @@
 ///  scratch buffer.  If the ContentCache encapsulates a source file, that
 ///  file is not lazily brought in from disk to satisfy this query.
 unsigned ContentCache::getSize() const {
-  return Buffer.getPointer() ? (unsigned) Buffer.getPointer()->getBufferSize()
- : (unsigned) ContentsEntry->getSize();
-}
-
-void ContentCache::replaceBuffer(const llvm::MemoryBuffer *B, bool DoNotFree) {
-  IsBufferInvalid = false;
-
-  if (B && B == Buffer.getPointer()) {
-assert(0 && "Replacing with the same buffer");
-Buffer.setInt(DoNotFree? DoNotFreeFlag : 0);
-return;
-  }
-
-  if (shouldFreeBuffer())
-delete Buffer.getPointer();
-  Buffer.setPointer(B);
-  Buffer.setInt((B && DoNotFree) ? DoNotFreeFlag : 0);
+  return Buffer ? (unsigned)Buffer->getBufferSize()
+: (unsigned)ContentsEntry->getSize();
 }
 
 llvm::Optional
@@ -102,10 +81,10 @@
 SourceLocation Loc) const {
   // Lazily create the Buffer for ContentCaches that wrap files.  If we already
   // computed it, just return what we have.
-  if (isBufferInvalid())
+  if (IsBufferInvalid)
 return None;
-  if (auto *B = Buffer.getPointer())
-return B->getMemBufferRef();
+  if (Buffer)
+return Buffer->getMemBufferRef();
   if (!ContentsEntry)
 return None;
 
@@ -147,7 +126,7 @@
 return None;
   }
 
-  Buffer.setPointer(BufferOrError->release());
+  Buffer = std::move(*BufferOrError);
 
   // Check that the file's size is the same as in the file entry (which may
   // have come from a stat cache).
@@ -166,7 +145,7 @@
   // If the buffer is valid, check to see if it has a UTF Byte Order Mark