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