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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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;
----------------
oontvoo wrote:
> 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?)
> 
Hmm, think maybe there's some confusion - could we keep the first error and stop there, returning the error immediately, rather than tracking it through the function?


================
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;
----------------
oontvoo wrote:
> 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.
> 
hmm - consistency with what? We could say that if this function produces an Error the argv should not be used - that the caller should error out immediately and not try to parse anything else from argv.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1267-1271
+  if (ExpandResponseFilesWithError(Saver, std::move(Tokenizer), Argv, MarkEOLs,
+                                   RelativeNames, std::move(CurrentDir),
+                                   *vfs::getRealFileSystem()))
+    return false;
+  return true;
----------------
dblaikie wrote:
> Looks like this'll result in an assert if the function returns a non-success error, it probably needs a consumeError (or whatever it's called) in the failure case before returning false.
^ this is still pending/unaddressed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110905