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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo marked 6 inline comments as done.
oontvoo added inline comments.


================
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:
> 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.
fair enough!


================
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:
> 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
done (for real this time)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110905