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