This is an archive of the discontinued LLVM Phabricator instance.

[llvm-size] Tidy up error messages
ClosedPublic

Authored by tetsuo-cpp on Oct 11 2019, 11:30 PM.

Details

Summary

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=42970

This patch is to clean up some formatting inconsistencies in the error messages of llvm-size and to correctly exit with non-zero in all error cases (there were a few which were exiting with zero). When working on the error messages, I based the formatting off the way that llvm-objdump does it.

Diff Detail

Event Timeline

tetsuo-cpp created this revision.Oct 11 2019, 11:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2019, 11:30 PM
MaskRay added inline comments.Oct 12 2019, 1:14 AM
llvm/tools/llvm-size/llvm-size.cpp
532

No -> no

Most llvm binary utilities have switched to lowercase error messages.

MaskRay added inline comments.Oct 12 2019, 1:15 AM
llvm/tools/llvm-size/llvm-size.cpp
110

I'd prefer error(Twine Message, StringRef File). Note the error below places the error (for error message) at the first argument position.

grimar added inline comments.Oct 12 2019, 3:24 AM
llvm/tools/llvm-size/llvm-size.cpp
110

Yeah. But it should be const Twine &
("Twines should only be used accepted as const references in arguments"
https://llvm.org/doxygen/classllvm_1_1Twine.html#details)

Also, why does it return bool? Can it be void?

Address review comments.

tetsuo-cpp marked 3 inline comments as done.Oct 12 2019, 5:44 AM
MaskRay accepted this revision.Oct 12 2019, 7:10 AM

LGTM, but please wait for @grimar's opinion.

This revision is now accepted and ready to land.Oct 12 2019, 7:10 AM
grimar accepted this revision.Oct 14 2019, 2:21 AM

LGTM.

llvm/tools/llvm-size/llvm-size.cpp
113

FTR, I think error stream is unbuffered, i.e. no need to flush(), but it is probably unrelated to this patch.

Remove unnecessary flush.

tetsuo-cpp marked 2 inline comments as done.Oct 14 2019, 3:27 AM

Thanks for the reviews @MaskRay and @grimar.
If you're still happy with this patch, could one of you please commit on my behalf?

llvm/tools/llvm-size/llvm-size.cpp
113

Yep, it looks like it is indeed unbuffered so I've removed the flush.

tetsuo-cpp marked an inline comment as done.Oct 14 2019, 3:35 AM

Thanks for the reviews @MaskRay and @grimar.
If you're still happy with this patch, could one of you please commit on my behalf?

I am off today, but can do this tomorrow. If this will not happen earlier :]

This revision was automatically updated to reflect the committed changes.

Thanks, Fangrui!

thakis added a subscriber: thakis.Oct 14 2019, 6:29 AM

This breaks check-llvm on mac, Linux, and windows. Can you please revert?

Reverted in r374780.

Reverted in r374780.

Many thanks for keeping the build green -- I've updated the failing test case (check-llvm, lld, clang all pass) & recommitted this in r374793.

I'm sorry for causing trouble. I was only running check-llvm-tools so I didn't notice this problem. I'll make sure I run everything in the future.