[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/COFFObjcopy.cpp:183
 
-  // Actually do removals of symbols.
-  Obj.removeSymbols([&](const Symbol &Sym) {
+  std::function<Expected<bool>(const Symbol &)> ToRemove =
+      [&](const Symbol &Sym) -> Expected<bool> {
----------------
I think lambdas are one of the exceptions to the "don't use `auto` rule", so you can simpify this to `auto ToRemove`. Of course, you could also just leave it inline it like it was before, but I don't mind if you'd prefer to keep it separate.


================
Comment at: llvm/tools/llvm-objcopy/COFF/Object.cpp:63
+  if (LastRemoved)
+    Symbols.erase(*LastRemoved, Symbols.end());
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88213