[llvm-ar] Removes repetition in the error message

[wyjw]

Summary

As per bug 40244, fixed an error where the error message was repeated.

[wyjw]

This comment was removed by wyjw.

[wyjw]

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

[jhenderson]

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]

Renamed test file, changed after the comments.

[wyjw]

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.

2. 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]

cleaned up test

[jhenderson]

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]

Made changes to wording and to the test file.

[jhenderson]

Thanks, LGTM.

[MaskRay]

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]

remove a period

[wyjw]

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

Thanks!

[MaskRay]

In D67038#1675878, @wyjw wrote:

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.

llvm/test/tools/llvm-ar/invalid-object-file.test

+## Test that we get a proper error message with a file that is not a recognized object file.
+
+RUN: touch %t1.txt
+RUN: not llvm-ar sc %t1.txt 2>&1 > /dev/null | FileCheck -allow-empty %s
+
+CHECK: llvm-ar{{(.exe|.EXE)?}}: error: error loading '{{[^']+}}': File too small to be an archive.
+CHECK-NOT: {{.}}
+
+RUN: rm %t1.txt

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

static int performOperation(ArchiveOperation Operation,
   ErrorOr<std::unique_ptr<MemoryBuffer>> Buf =
       MemoryBuffer::getFile(ArchiveName, -1, false);
   std::error_code EC = Buf.getError();
   if (EC && EC != errc::no_such_file_or_directory)
     fail("error opening '" + ArchiveName + "': " + EC.message() + "!");
 
   if (!EC) {
     Error Err = Error::success();
     object::Archive Archive(Buf.get()->getMemBufferRef(), Err);
-    EC = errorToErrorCode(std::move(Err));
-    failIfError(EC,
-                "error loading '" + ArchiveName + "': " + EC.message() + "!");
+    failIfError(std::move(Err), "error loading '" + ArchiveName + "'");
     if (Archive.isThin())
       CompareFullPath = true;
     performOperation(Operation, &Archive, std::move(Buf.get()), NewMembers);
     return 0;
   }
 
   assert(EC == errc::no_such_file_or_directory);