Page MenuHomePhabricator

[llvm-objcopy] Improve error message for unrecognised archive member
ClosedPublic

Authored by jhenderson on May 8 2019, 4:03 AM.

Details

Summary

Prior to this patch, llvm-objcopy's error messages for archives with unsupported members only mentioned the archive name, not the member name, making them unhelpful. This change improves it by approximately following GNU objcopy's error message syntax of "<archive name>(<member name>): <problem>".

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.May 8 2019, 4:03 AM
Herald added a reviewer: alexshap. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
grimar added inline comments.May 8 2019, 5:35 AM
test/tools/llvm-objcopy/ELF/archive-unknown-members.test
2 ↗(On Diff #198620)

unrecognised -> unrecognized I think?

26 ↗(On Diff #198620)

You have %t.thin1.a %t2.a for llvm-objcopy call and just %t.thin2.a for llvm-strip.
I think you placed %t2.a by mistake?

tools/llvm-objcopy/llvm-objcopy.cpp
165 ↗(On Diff #198620)

nit: while you are here: I would perhaps remove this variable since it is used only once.

jhenderson updated this revision to Diff 198635.May 8 2019, 5:57 AM

Address review comments.

jhenderson marked 5 inline comments as done.May 8 2019, 5:57 AM
jhenderson added inline comments.
test/tools/llvm-objcopy/ELF/archive-unknown-members.test
2 ↗(On Diff #198620)

Thanks. Stupid American spelling ;)

26 ↗(On Diff #198620)

Oops, thanks.

grimar accepted this revision.May 8 2019, 5:58 AM

LGTM

This revision is now accepted and ready to land.May 8 2019, 5:58 AM
jhenderson marked 2 inline comments as done.May 8 2019, 5:59 AM

Thanks @grimar!

grimar added inline comments.May 8 2019, 6:00 AM
test/tools/llvm-objcopy/ELF/archive-unknown-members.test
31 ↗(On Diff #198635)

recognised->recognized :]

This revision was automatically updated to reflect the committed changes.

Sorry, I didn't see your latest comment before committing. I've fixed it in a follow up change.

MaskRay added inline comments.May 8 2019, 6:30 AM
test/tools/llvm-objcopy/ELF/archive-unknown-members.test
31 ↗(On Diff #198635)

recognised is not wrong..

grimar added inline comments.May 8 2019, 6:41 AM
test/tools/llvm-objcopy/ELF/archive-unknown-members.test
31 ↗(On Diff #198635)

As far I understand there is a difference between British and American English.
Since our error messages spelling "recognized", i.e. in American way, I think it is good for
comments to be consistent. See rL360252

jhenderson marked an inline comment as done.May 8 2019, 6:46 AM
jhenderson added inline comments.
test/tools/llvm-objcopy/ELF/archive-unknown-members.test
31 ↗(On Diff #198635)

I thought I read somewhere that the standard was American English in LLVM, but I now can't find it, so maybe I imagined it?

grimar added inline comments.May 8 2019, 6:59 AM
test/tools/llvm-objcopy/ELF/archive-unknown-members.test
31 ↗(On Diff #198635)

Honestly saying I suggested to change unrecognised -> unrecognized initially because my spellchecker complained about this word. I realized that it is a British style only after your first reply :) I do not know what is the LLVM standard (I would not expect to see such a requirement probably.)

Having consistency in spelling is not that bad still I think. I.e. in this test error messages spelling wasn't consistent with the comments before and now it is. But perhaps it is really was too subtle to care about?