This is an archive of the discontinued LLVM Phabricator instance.

[llvm-ar] Modify usage printouts to use the correct toolname
ClosedPublic

Authored by chrisjackson on Apr 26 2022, 4:20 AM.

Details

Summary

When using 'llvm-ar --help' or 'llvm-ranlib --help' the toolname is currently hardcoded e.g. 'USAGE: llvm-ar ....' . Therefore if a custom toolchain is used the usage is in fact incorrect, 'custom-llvm-ar --help' will still display 'USAGE: llvm-ar'.

This patch brings the usage toolname printout in line with other tools such as llvm-objdump and llvm-readobj. However, the patch stops well short of converting the help options to tablegen, which would bring llvm-ar fully inline with the other tool implementations and remove the string literals for the options altogether.

Diff Detail

Event Timeline

chrisjackson created this revision.Apr 26 2022, 4:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 4:20 AM
Herald added a subscriber: rupprecht. · View Herald Transcript
chrisjackson requested review of this revision.Apr 26 2022, 4:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 4:20 AM
chrisjackson edited the summary of this revision. (Show Details)Apr 26 2022, 4:20 AM
chrisjackson edited the summary of this revision. (Show Details)Apr 26 2022, 4:38 AM
thopre added a subscriber: thopre.Apr 26 2022, 5:13 AM

This looks like a good change to me, just needs that ranlib test fixing.

Looks good to me.

llvm/tools/llvm-ar/llvm-ar.cpp
66

printArHelp omits ToolName on the OVERVIEW line. Omit it here?

Two test nits, otherwise looks fine to me.

llvm/test/tools/llvm-ar/tool-name.test
15–17

You should add something like {{ }} at the end of these lines, to show that there is nothing else after the tool name. In particular, whether .exe is present in the SUFFIX case.

18

What's the purpose of this line? To show that the prefix is printed as much as the suffix, or something more specific?

Also, it's missing "USAGE:" unlike the other patterns.

chrisjackson marked 2 inline comments as done.

Respond to reviewer comments:

  • Add missed llvm-ranlib/tool-name.test update
  • Make ranlib printout consistent with ar
  • Make toolname pattern check more strict
llvm/test/tools/llvm-ar/tool-name.test
18

looking at https://github.com/llvm/llvm-project/commit/4d53b99c5df2c83172d23521d1b1ab8361d6de92, this exists because the tool selection heuristic would mistakenly think it was 'llvm-lib', so I have retained this check.

llvm/tools/llvm-ar/llvm-ar.cpp
66

Agreed, removed for consistency,

chrisjackson added inline comments.Apr 27 2022, 9:47 AM
llvm/test/tools/llvm-ar/tool-name.test
18

Actually, thinking about it, this check no longer guarantees that the correct tool has been chosed. A different line from the usage output that contains the default tool name will have to be checked

MaskRay accepted this revision.Apr 27 2022, 10:04 AM

LGTM once @jhenderson's comments are addressed.

This revision is now accepted and ready to land.Apr 27 2022, 10:04 AM
chrisjackson added inline comments.Apr 29 2022, 1:26 AM
llvm/test/tools/llvm-ar/tool-name.test
18

On second thoughts, I think my original response stands. @jhenderson please can you confirm if you are you happy with this explanation?

jhenderson accepted this revision.Apr 29 2022, 1:46 AM

LGTM, with suggestion.

llvm/test/tools/llvm-ar/tool-name.test
18

I think it's fine, but might warrant a brief comment saying the purpose of the check.

Address reviewer comments by adding a clarification in the llvm-ar tool-name test.

jhenderson added inline comments.May 3 2022, 7:48 AM
llvm/test/tools/llvm-ar/tool-name.test
13

I'd quote the "lib" word, as per the suggested edit: this should make it clear that "lib" is actually the substring, and not that there's some "lib substring".

This revision was landed with ongoing or failed builds.May 3 2022, 9:45 AM
This revision was automatically updated to reflect the committed changes.