[PATCH] D65534: [clang] Change FileManager to use llvm::ErrorOr instead of null on failure

[Harlan Haskins via Phabricator via cfe-commits]

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