This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.May 8 2019, 4:03 AM
grimar added inline comments.May 8 2019, 5:35 AM
test/tools/llvm-objcopy/ELF/archive-unknown-members.test
3

unrecognised -> unrecognized I think?

27

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

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
3

Thanks. Stupid American spelling ;)

27

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

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

recognised is not wrong..

grimar added inline comments.May 8 2019, 6:41 AM
test/tools/llvm-objcopy/ELF/archive-unknown-members.test
31

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

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

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?