Page MenuHomePhabricator

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

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



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


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

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.
2 ↗(On Diff #198620)

Thanks. Stupid American spelling ;)

26 ↗(On Diff #198620)

Oops, thanks.

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


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
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
31 ↗(On Diff #198635)

recognised is not wrong..

grimar added inline comments.May 8 2019, 6:41 AM
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.
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
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?