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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/COFF/Object.cpp:63
+  if (LastRemoved)
+    Symbols.erase(*LastRemoved, Symbols.end());
+
----------------
jhenderson wrote:
> 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.
The iteration logic seems changed. Version from the current patch will stop iterating as soon as first error encountered(the same as without patch). The suggested version would iterate all Symbols and accumulate the errors. But that is probably OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88213