[PATCH] D67038: Removes repetition in the error message.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: gbreynoo.
jhenderson added a comment.

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)?



================
Comment at: llvm/test/tools/llvm-ar/invalid-binary-file.test:1
+Test if we get a proper error message with a file that is not a proper binary.
+
----------------
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


================
Comment at: llvm/test/tools/llvm-ar/invalid-binary-file.test:7
+CHECK: llvm-ar{{(.exe|.EXE)?}}: error: error loading '{{[^']+}}': The file was not recognized as a valid object file.
+CHECK-NOT: The file
----------------
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.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:930-931
     object::Archive Archive(Buf.get()->getMemBufferRef(), Err);
     EC = errorToErrorCode(std::move(Err));
     failIfError(EC,
+                "error loading '" + ArchiveName + "'");
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67038/new/

https://reviews.llvm.org/D67038