[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1148
+      llvm::ErrorOr<llvm::vfs::Status> RHS = FS.status(RFile.File);
+      if (!LHS || !RHS) {
+        return false;
----------------
kadircet wrote:
> again you need to `consumeError`s before returning. I would first get `LHS`, consume and bail out if it was an error, then do the same for `RHS` and only after that return `equivalent`.
Could you add a TODO comment here and at the other `consumeError()` call to say that the error should be propagated up the stack, please?


================
Comment at: llvm/lib/Support/CommandLine.cpp:1054
+  if (!CurrDirOrErr) {
+    llvm::consumeError(llvm::errorCodeToError(CurrDirOrErr.getError()));
     return false;
----------------
I'd ultimately like to see the public interface return llvm::Expected/llvm::Error so that a potentially useful error with information isn't just thrown away. However, I agree that that's probably a separate change. On the other hand, it should be simple to return it here, since this is only used locally, for consumption in `ExpandResponseFiles` below. Could you make that change, please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769