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

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

dexonsmith 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 =
----------------
arphaman wrote:
> 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?
I agree the main file should really be there. But I'd rather keep this patch as NFC as possible, so I think not in this commit. I also don't see an easy way to add that assertion in a prep commit.

WDYT of doing it in a follow-up?


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


================
Comment at: clang/lib/Basic/SourceManager.cpp:1179
+    *Invalid = !Buffer;
+  return Buffer ? Buffer->getBufferStart() + LocInfo.second : nullptr;
 }
----------------
arphaman wrote:
> 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.
Good question, but I don't think we need to care since this is NFC. See the old `return` statement:
```
return Buffer->getBufferStart() + (CharDataInvalid? 0 : LocInfo.second);
```


================
Comment at: clang/lib/Basic/SourceManager.cpp:1467
+    return Buffer->getBufferIdentifier();
+  return "";
 }
----------------
arphaman wrote:
> Should you return `"<invalid buffer>"` here to be consistent with the `"invalid loc"` above?
To keep this NFC, this should have the same identifier that the old `getBuffer` call would have had. I'll double check it was empty before.


================
Comment at: clang/lib/Basic/SourceManager.cpp:1706
+      Content->getBuffer(Diag, getFileManager());
   unsigned FilePos = Content->SourceLineCache[Line - 1];
   const char *Buf = Buffer->getBufferStart() + FilePos;
----------------
arphaman wrote:
> 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.
That's a good point.

It actually adds a concern for me, since this patch is fairly old (rebased after over a year). Could there be calls to `getBuffer` that I've missed?

I can do a more careful audit.

There's a way to do this more safely: adding a new API with a different name, migrating everyone over, deleting the old API, and then renaming the new API. However, I'm concerned that will create even more churn. Do you think that would be better?


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

https://reviews.llvm.org/D66782