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

Thu Aug 1 12:47:47 PDT 2019

jkorous accepted this revision.
jkorous marked an inline comment as done.
jkorous added a comment.
This revision is now accepted and ready to land.

LGTM

Thanks for all the work here!

================
Comment at: clang/lib/ARCMigrate/FileRemapper.cpp:156
+      auto newE = FileMgr->getFile(tempPath);
+      if (newE) {
+        remap(origFE, *newE);
----------------
harlanhaskins wrote:
> 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?
To be clear - I didn't have any opinion, just noticed there's a functional change and pointed that out.
On the other hand I assume your solution is fine.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65534/new/

https://reviews.llvm.org/D65534