[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;
----------------
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.


================
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;
----------------
Can this return immediately with th eerror, rather than setting the error and continuing to do more work?


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110905