Page MenuHomePhabricator

[llvm-readobj/llvm-readelf] - Improve/cleanup the error reporting API.
ClosedPublic

Authored by grimar on Aug 15 2019, 4:13 AM.

Details

Summary

Currently we have the following functions for error reporting:

LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg);
void reportError(Error Err, StringRef Input); 
void reportWarning(Twine Msg);
void reportWarning(StringRef Input, Error Err);
void warn(llvm::Error Err);
void error(std::error_code EC);

Problems are: naming is inconsistent, arguments order is inconsistent,
some of the functions looks excessive.

After applying this patch we have:

void reportError(Error Err, StringRef Input); 
void reportError(std::error_code EC, StringRef Input);
void reportWarning(Error Err, StringRef Input);

I'd be happy to remove reportError(std::error_code EC, StringRef Input) too, but it
is used by COFF heavily.

Test cases were updated, they show an improvement introduced.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Aug 15 2019, 4:13 AM
jhenderson added inline comments.Aug 15 2019, 8:57 AM
tools/llvm-readobj/ObjDumper.cpp
27 ↗(On Diff #215367)

Can you use createStringError here?

MaskRay added inline comments.Aug 15 2019, 9:17 AM
tools/llvm-readobj/ELFDumper.cpp
4623 ↗(On Diff #215367)

I think if a variable already exists, keeping it probably doesn't hurt. Typing Obj->getFileName() takes a bit more characters.

rupprecht added inline comments.Aug 15 2019, 11:12 AM
tools/llvm-readobj/ELFDumper.cpp
141–144 ↗(On Diff #215367)

I think <?> is confusing, and is worth fixing in this patch instead of deferring to a followup cleanup.

Could you perhaps make DynRegionInfo take a required filename parameter which can be initialized in ELFDumper's constructor so it's always available?

tools/llvm-readobj/llvm-readobj.cpp
400 ↗(On Diff #215367)

... any idea why this was necessary before? This is a different stream. Might be nice to have a comment in the code. (Same for above in error()).

tools/llvm-readobj/llvm-readobj.h
24–25 ↗(On Diff #215367)

Can these both be LLVM_ATTRIBUTE_NORETURN? I think they should have been so before too.

grimar updated this revision to Diff 215557.Aug 16 2019, 3:27 AM
grimar marked 11 inline comments as done.
  • Addressed review comments.
tools/llvm-readobj/ELFDumper.cpp
141–144 ↗(On Diff #215367)

Ok, done.
(FTR, "<?>" is used in COFFDumper.cpp, I took the idea from there.)

4623 ↗(On Diff #215367)

Ok.

tools/llvm-readobj/ObjDumper.cpp
27 ↗(On Diff #215367)

Done.

tools/llvm-readobj/llvm-readobj.cpp
400 ↗(On Diff #215367)

... any idea why this was necessary before? This is a different stream.

Yes, I added it in D64472 because otherwise warnings reported could be printed at the wrong places.
(i.e. they were kind of randomly mixed to the standart output).

I added comments requested.

tools/llvm-readobj/llvm-readobj.h
24–25 ↗(On Diff #215367)

Done.

MaskRay added inline comments.Aug 16 2019, 3:38 AM
tools/llvm-readobj/llvm-readobj.cpp
392 ↗(On Diff #215557)

llvm_unreachable after handleAllErrors. handleAllErrors is not marked noreturn, so it needs manual annotation.

grimar updated this revision to Diff 215570.Aug 16 2019, 5:22 AM
grimar marked an inline comment as done.
  • Addressed review comment.
rupprecht accepted this revision.Aug 16 2019, 11:25 AM
rupprecht added inline comments.
tools/llvm-readobj/llvm-readobj.cpp
379 ↗(On Diff #215570)

standart -> standard

403 ↗(On Diff #215570)

standart -> standard

This revision is now accepted and ready to land.Aug 16 2019, 11:25 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2019, 7:35 AM
Herald added a subscriber: jrtc27. · View Herald Transcript