[PATCH] D88213: [llvm-objcopy][NFC] refactor error handling. part 2.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 25 00:38:48 PDT 2020
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
More information about the llvm-commits
mailing list