[PATCH] D57462: [llvm-objcopy][NFC] More error propagation (executeObjcopyOnArchive)

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked 3 inline comments as done.
rupprecht added a comment.

In D57462#1380244 <https://reviews.llvm.org/D57462#1380244>, @jhenderson wrote:

> In D57462#1379689 <https://reviews.llvm.org/D57462#1379689>, @dblaikie wrote:
>
> > Any new test coverage for these new error propagation paths?
>
>
> I believe that there are actually no new errors that can be produced. They're just reported further up the stack rather than deep inside the llvm-objcopy code. Most (all?) of the error cases that can be tested should already be being tested.


Yep, these aren't new errors. And specifically tools/llvm-objcopy/ELF/strip-debug.test is what's requiring the cantFail() check, so we can use that as a test to make sure the iterator is fixed.



================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:157
+    // iteration, and hence does not allow early returns.
+    (void)(bool) Err;
     Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
----------------
dblaikie wrote:
> jhenderson wrote:
> > I think this should be `cantFail(Err)`, right?
> Oh, good call!
I think cantFail() is meant to wrap calls directly, e.g. `cantFail(doSomething())`, as it takes Error (which is uncopyable) by value (i.e. move-only).

So that would have to be `cantFail(std::move(Err))`. Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57462