This is an archive of the discontinued LLVM Phabricator instance.

[llvm-ar] Uncapitalize and delete full stop from error messages
ClosedPublic

Authored by MaskRay on Sep 13 2019, 9:05 AM.

Details

Summary

Most GNU binutils don't append full stops in error messages. This
convention has been adopted by a bunch of LLVM binary utilities. Make
llvm-ar follow the convention as well.

Diff Detail

Event Timeline

MaskRay created this revision.Sep 13 2019, 9:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 9:05 AM
grimar added inline comments.Sep 13 2019, 9:10 AM
tools/llvm-ar/llvm-ar.cpp
1139 ↗(On Diff #220118)

Should we remove ! too?

grimar added inline comments.Sep 13 2019, 9:15 AM
tools/llvm-ar/llvm-ar.cpp
386 ↗(On Diff #220118)

btw, below modifiers are sometimes wrapped into ''. May be worth to make this consistent in messages while you are here too?

MaskRay updated this revision to Diff 220122.Sep 13 2019, 9:17 AM
MaskRay marked an inline comment as done.

Delete !

MaskRay updated this revision to Diff 220125.Sep 13 2019, 9:24 AM
MaskRay marked an inline comment as done and an inline comment as not done.

Wrap operation names in ''

tools/llvm-ar/llvm-ar.cpp
386 ↗(On Diff #220118)

Wrapped in '' for consistency.

It seems many error messages here are no tested. This is something we should improve in the future.

grimar accepted this revision.Sep 13 2019, 9:31 AM

LGTM with a nits.

tools/llvm-ar/llvm-ar.cpp
212 ↗(On Diff #220125)

nit: needs wrapping too it seems.

221 ↗(On Diff #220125)

The same.

386 ↗(On Diff #220118)

Yeah. I also found one more inconsistency:

"cannot mix -M and other options"

Seems in other places we use the word "operation", but here the word "option" with a dash.
It is anyways out of the area of this patch, just had to mention :)

This revision is now accepted and ready to land.Sep 13 2019, 9:31 AM
MaskRay updated this revision to Diff 220129.Sep 13 2019, 9:44 AM
MaskRay marked 5 inline comments as done.

Wrap two other error messages

This revision was automatically updated to reflect the committed changes.