[PATCH] D89429: clang/Basic: Replace SourceManager::getMemoryBufferForFile, NFC

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

dexonsmith added inline comments.


================
Comment at: clang/lib/Basic/SourceManager.cpp:121-127
 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;
 }
----------------
[Highlighting this for the other comment.]


================
Comment at: clang/lib/Basic/SourceManager.cpp:705
+llvm::Optional<llvm::MemoryBufferRef>
+SourceManager::getMemoryBufferForFileOrNone(const FileEntry *File) {
   const SrcMgr::ContentCache *IR = getOrCreateContentCache(File);
----------------
JDevlieghere wrote:
> Would it make sense to rename this to the slightly less verbose `getMemoryBufferOrNone` now that it takes just the File as an argument? Or maybe even `getBufferOrNone` like the method in `ContentCache`. 
There's already a `SourceManager::getBufferOrNone` (see line 121 of this file, which I am highlighting). I'd rather keep the current weird naming to make it easy to grep for and eventually delete.

This function shouldn't really exist since it's at the wrong layer (`FileEntry*` -> `MemoryBuffer` is a `FileManager` concept). The problem is that the `SourceManager` owns the memory buffer so this is the only way to get access to it. Once I sink the content cache / `MemoryBuffer` ownership into the `FileManager`, we can delete this entirely.


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

https://reviews.llvm.org/D89429