[PATCH] D92318: [llvm-readobj, libSupport] - Refine the implementation of the code that dumps build attributes.
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 2 01:33:27 PST 2020
grimar added inline comments.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2919-2920
- // TODO: Delete the redundant FormatVersion check above.
- if (Machine == EM_ARM) {
- if (Error E = ARMAttributeParser(&W).parse(Contents, support::little))
- reportWarning(std::move(E), ObjF.getFileName());
- } else if (Machine == EM_RISCV) {
- if (Error E = RISCVAttributeParser(&W).parse(Contents, support::little))
- reportWarning(std::move(E), ObjF.getFileName());
- }
+ Error E = Error::success();
+ consumeError(std::move(E));
+ if (Machine == EM_ARM)
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > I think you can avoid this dance using the Error as output parameter idiom. See `ErrorAsOutParameter`.
> > I am not sure I understood tthe idea. Are you saying that `parse` should accept an `Error` argument?
> > `ErrorAsOutParameter` clears the checked bit in destructor. I wonder how it might help here.
> >
> > I've replaced `consumeError` with `cantFail`.
> Never mind, I misread the code. However, I have an alternative idea now: factor the `if (Machine == EM_ARM) ...` if statement and its corresponding else into a separat function/lambda, then return the Error so that you can do:
>
> ```
> Error E = parseAttributes(...);
> if (E)
> reportUniqueWarning(...);
> ```
>
> What do you think?
Done.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92318/new/
https://reviews.llvm.org/D92318
More information about the llvm-commits
mailing list