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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
