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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, but please make sure others are happy too.



================
Comment at: llvm/tools/llvm-objcopy/COFF/Object.cpp:63
+  if (LastRemoved)
+    Symbols.erase(*LastRemoved, Symbols.end());
+
----------------
avl wrote:
> 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.
Yes, I think it's probably fine. If there was a simple way to stop collecting errors and bail out after a certain point, that would also be fine, but I think the code is better off like this otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88213