[PATCH] D110905: [llvm] Add overload to propagate errors up to caller from ExpandResponseFiles
Vy Nguyen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 4 12:29:38 PDT 2021
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
More information about the llvm-commits
mailing list