[PATCH] D110905: [llvm] Add overload to propagate errors up to caller from ExpandResponseFiles

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1196-1197
       if (!LHS) {
-        // TODO: The error should be propagated up the stack.
-        llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
+        if (!FirstError)
+          FirstError = llvm::errorCodeToError(LHS.getError());
         return false;
----------------
dblaikie wrote:
> This "!FirstError / FirstError = "  idiom seems like one that'd be nice to avoid. I guess it's mostly necessary because FirstError starts with success, and assigning it a new value immediately would cause an assertion about the error not being tested before being replaced.
> 
> Rather than using that idiom - perhaps the function could early return instead?  This lambda could return Expected<bool>? Oh, nope, because it's used in any_of. /might/ be worth expanding that out/not using any_of to simplify the error handling in some ways I think.
Right, I'd like to keep the first error (because usually it's the more useful than subsequent ones).

How about this - we can have it queue up all the errors and return them? (Rather than just the first one?)



================
Comment at: llvm/lib/Support/CommandLine.cpp:1232-1233
       // move on.
-      // TODO: The error should be propagated up the stack.
-      llvm::consumeError(std::move(Err));
+      if (!FirstError)
+        FirstError = std::move(Err);
       AllExpanded = false;
----------------
dblaikie wrote:
> Can this return immediately with th eerror, rather than setting the error and continuing to do more work?
No, because we should clean up Argv and ExpandedArgv (code below) to ensure consistency.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110905