[PATCH] D66782: SourceManager: Prefer Optional<MemoryBufferRef> over MemoryBuffer*

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:459
                                                         Position P) {
-  llvm::StringRef Code = SM.getBuffer(SM.getMainFileID())->getBuffer();
+  llvm::StringRef Code = SM.getBufferOrFake(SM.getMainFileID()).getBuffer();
   auto Offset =
----------------
I feel like all buffer requests for the main file should succeed and shouldn't need the fake buffer, unless something gone terribly wrong, Do you agree? Do you think it might be valuable to add a method like `SM.getMainFileBuffer` which has an `llvm_unreachable` if buffer is invalid?


================
Comment at: clang/include/clang/Basic/SourceManager.h:303
 
+    static FileInfo getForRecovery() {
+      FileInfo X = get(SourceLocation(), nullptr, SrcMgr::C_User, "");
----------------
This doesn't seem to be used in this patch, is this dead code?


================
Comment at: clang/lib/Basic/SourceLocation.cpp:253
+  if (Buffer)
+    return Buffer->getBuffer();
+  return "";
----------------
Can be simplified to `return Buffer ? Buffer->getBuffer() : ""`


================
Comment at: clang/lib/Basic/SourceManager.cpp:1179
+    *Invalid = !Buffer;
+  return Buffer ? Buffer->getBufferStart() + LocInfo.second : nullptr;
 }
----------------
How come you're returning `nullptr` here instead of `"<<<<INVALID BUFFER>>>>"` like in the error condition above? It seems that clients will not be able to handle this nullptr.


================
Comment at: clang/lib/Basic/SourceManager.cpp:1467
+    return Buffer->getBufferIdentifier();
+  return "";
 }
----------------
Should you return `"<invalid buffer>"` here to be consistent with the `"invalid loc"` above?


================
Comment at: clang/lib/Basic/SourceManager.cpp:1698
+    if (llvm::Optional<unsigned> RawSize =
+            Content->getBufferSize(Diag, getFileManager()))
+      if (*RawSize > 0)
----------------
NIT: Please use `{}` for the outer `if`


================
Comment at: clang/lib/Basic/SourceManager.cpp:1706
+      Content->getBuffer(Diag, getFileManager());
   unsigned FilePos = Content->SourceLineCache[Line - 1];
   const char *Buf = Buffer->getBufferStart() + FilePos;
----------------
It appears you're not checking if `Buffer` is `None` here. Previously this should've been fine as it seems `getBuffer` created fake recovery buffers.


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

https://reviews.llvm.org/D66782