[PATCH] D88213: [llvm-objcopy][NFC] refactor error handling. part 2.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/COFF/Object.cpp:63
+  if (LastRemoved)
+    Symbols.erase(*LastRemoved, Symbols.end());
+
----------------
jhenderson wrote:
> mstorsjo wrote:
> > This implementation looks ok, but it's not entirely trivial - hopefully the tests cover all relevant cases here.
> > 
> > Does this happen to be similar to what the implementation of `std::remove_if` looks like?
> Similar comment - there might be a simpler way than this:
> ```
> Error Errs = Error::success();
> Symbols.erase(std:::remove_if(std::begin(Symbols), std::end(Symbols),
>               [ToRemove, &Errs](const Symbol &Sym) {
>     Expected<bool> ShouldRemove = ToRemove(Sym);
>     if (ShouldRemove) {
>       Errs = joinErrors(std::move(Errs), ShouldRemove.takeError());
>       return false;
>     }
>     return *ShouldRemove;
>   }), std::end(Symbols));
> return Errs;
> ```
> (I haven't tested for syntax and exact usage errors, but this should be approximately correct). Essentially, we intercept the errors from the `ToRemove` function, join them all together, and then pass them all up the stack. The iteration logic remains unchanged - we've just added error handling.
I missed the call to `updateSymbols` here - make sure to include that before the `return Errs;` line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88213