[Lldb-commits] [PATCH] D65534: [clang] Change FileManager to use llvm::ErrorOr instead of null on failure
Harlan Haskins via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Thu Aug 1 11:24:18 PDT 2019
harlanhaskins marked 6 inline comments as done and an inline comment as not done.
harlanhaskins added inline comments.
================
Comment at: clang/lib/ARCMigrate/FileRemapper.cpp:156
+ auto newE = FileMgr->getFile(tempPath);
+ if (newE) {
+ remap(origFE, *newE);
----------------
jkorous wrote:
> It seems like we're now avoiding the assert in `remap()` we'd hit previously. Is this fine?
If we want to trap if this temp file fails, then I'm happy removing the condition check. Do you think this should trap on failure or should it check the condition?
================
Comment at: clang/lib/ARCMigrate/FileRemapper.cpp:229
const FileEntry *FileRemapper::getOriginalFile(StringRef filePath) {
- const FileEntry *file = FileMgr->getFile(filePath);
+ const FileEntry *file;
+ if (auto fileOrErr = FileMgr->getFile(filePath))
----------------
jkorous wrote:
> Shouldn't we initialize this guy to `nullptr`?
👍 good catch!
================
Comment at: clang/lib/Frontend/FrontendAction.cpp:715
SmallString<128> DirNative;
- llvm::sys::path::native(PCHDir->getName(), DirNative);
+ if (*PCHDir == nullptr) {
+ llvm::dbgs() << "Directory " << PCHInclude << " was null but no error thrown";
----------------
jkorous wrote:
> Could this actually happen? I was expecting the behavior to be consistent with `getFile()`.
Oops, debugging code that needs to be removed. Thanks for catching it!
================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1448
if (getHeaderSearchOpts().ModuleMapFileHomeIsCwd)
- Dir = FileMgr.getDirectory(".");
+ Dir = *FileMgr.getDirectory(".");
else {
----------------
jkorous wrote:
> Seems like we're introducing assert here. Is that fine?
I'll make this conditional, thanks!
================
Comment at: clang/unittests/Basic/FileManagerTest.cpp:260
+
+ EXPECT_EQ(f1 ? *f1 : nullptr,
+ f2 ? *f2 : nullptr);
----------------
jkorous wrote:
> Just an idea - should we compare error codes?
Went ahead and added some tests ensuring we get useful error codes for a couple of common issues.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65534/new/
https://reviews.llvm.org/D65534
More information about the lldb-commits
mailing list