This is an archive of the discontinued LLVM Phabricator instance.

[llvm-ar] Removes repetition in the error message
ClosedPublic

Authored by wyjw on Aug 31 2019, 6:05 AM.

Details

Event Timeline

wyjw created this revision.Aug 31 2019, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2019, 6:05 AM
wyjw added a comment.Aug 31 2019, 6:20 AM
This comment was removed by wyjw.
wyjw updated this revision to Diff 218219.Aug 31 2019, 6:26 AM

Added test. The check-not is to make sure that the message is not repeated.

wyjw edited the summary of this revision. (Show Details)Aug 31 2019, 6:29 AM
wyjw edited the summary of this revision. (Show Details)
wyjw edited the summary of this revision. (Show Details)Aug 31 2019, 6:36 AM
wyjw added a reviewer: rupprecht.

Thanks for looking at this! Could you update the bug with a reference to this review, please? Do you plan on fixing the other message(s) in performOperation ("file not found" at least)?

llvm/test/tools/llvm-ar/invalid-binary-file.test
1 ↗(On Diff #218219)

Could you add '#' (or even '##' to be consistent with our new tests in the LLVM binutils) here, to make the comment stand out more, please?

Some wording improvement suggestions:
if -> that
propery binary -> recognized object file

7 ↗(On Diff #218219)

If you change this to CHECK-NOT: {{.}}, FileCheck will show that there is no output remaining at all. This ensures that you don't have other garbage at the end of the message (like an extra full stop or exclamation mark).

To go with this, you should probably add 'c' to the options passed to llvm-ar (i.e. make it 'llvm-ar rc') to suppress any possible archive creation message that might be printed.

llvm/tools/llvm-ar/llvm-ar.cpp
929–930

While you're here, how do you feel about getting rid of the errorToErrorCode call above and just passing in Err? I think the ultimate result is the same, but it saves a line of code, and allows for richer error messages (I see that the Archive constructor actually produces quite a number of semi-complex error messages that this code loses).

If you'd prefer not to do this now, or to do this and any other similar tidy-ups in a later change, that's fine with me.

wyjw updated this revision to Diff 220253.Sep 15 2019, 2:19 PM

Renamed test file, changed after the comments.

wyjw added a comment.EditedSep 15 2019, 2:24 PM

I have updated the patch after comments.

  1. If the file is not found, it does not have the same problem of the error repeated twice.
  1. Fail() calls printHelpMessage() and there seems to be no way to suppress that. I have attempted to pipe output to /dev/null (to get rid of the help message) but I am afraid that this might be Unix only.

Thank you for your time and help!

wyjw updated this revision to Diff 220254.Sep 15 2019, 2:37 PM

cleaned up test

jhenderson added inline comments.Sep 16 2019, 1:35 AM
llvm/test/tools/llvm-ar/invalid-object-file.test
4

Get rid of the -allow-empty. It's unnecessary, as an empty output is incorrect.

9

You don't need to do this here. However, at the start of the test, you should delete the file.

llvm/tools/llvm-ar/llvm-ar.cpp
930

Since this ends up as "error: error loading ...", perhaps change "error loading" to "unable to load".

wyjw updated this revision to Diff 220420.Sep 16 2019, 6:57 PM

Made changes to wording and to the test file.

jhenderson accepted this revision.Sep 17 2019, 2:00 AM

Thanks, LGTM.

This revision is now accepted and ready to land.Sep 17 2019, 2:00 AM
MaskRay added inline comments.
llvm/test/tools/llvm-ar/invalid-object-file.test
8

You'll have to rebase, error messages are no longer capitalized, i.e. File -> file

wyjw updated this revision to Diff 220782.Sep 18 2019, 6:11 PM
wyjw marked 4 inline comments as done.

remove a period

MaskRay accepted this revision.Sep 19 2019, 7:17 PM
MaskRay retitled this revision from Removes repetition in the error message. to [llvm-ar] Removes repetition in the error message.
wyjw marked an inline comment as done.Sep 19 2019, 7:50 PM

I do not have commit access. Could someone please help me with the commit?

Thanks!

I do not have commit access. Could someone please help me with the commit?

Thanks!

Thanks. Changed the test a bit and committed as r372370.

This revision was automatically updated to reflect the committed changes.