[PATCH] D110905: [llvm] Add overload to propagate errors up to caller from ExpandResponseFiles
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 1 11:16:53 PDT 2021
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
More information about the llvm-commits
mailing list