Page MenuHomePhabricator

[Docs][llvm-strip] Add help text to llvm-strip rst doc
ClosedPublic

Authored by mmpozulp on Jul 28 2019, 5:53 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mmpozulp created this revision.Jul 28 2019, 5:53 PM
Herald added a project: Restricted Project. · View Herald Transcript

Thanks for working on this! You're missing a number of aliases from the list. Please make sure they are included in the doc. Also, please use the text from the llvm-objcopy doc where the switch is common to the two tools.

llvm/docs/CommandGuide/llvm-strip.rst
2 ↗(On Diff #212130)

Delete the trailing '=' here.

9 ↗(On Diff #212130)

llvm-strip's usage is different to llvm-objcopy's. This follows llvm-objcopy's style. It should be something like :program:'llvm-strip' [*options*] *inputs...* (with back-ticks around llvm-strip instead of apostrophes, used due to not knowing how to escape them in Phabricator).

14 ↗(On Diff #212130)

This description should mention what happens by default, what the meaning of '-' is for input file names, and what happens for archive inputs (see the llvm-objcopy doc).

27 ↗(On Diff #212130)

This, and many of the other options use a more verbose/clearer description in the llvm-objcopy documentation for the same switch. I'd recommend you copy the text from llvm-objcopy's doc in each case, assuming it makes sense.

33–35 ↗(On Diff #212130)

This should be in the "ELF-specific" section.

40 ↗(On Diff #212130)

What about -h/--help?

47 ↗(On Diff #212130)

This should say something about what happens if there are multiple input files.

49–51 ↗(On Diff #212130)

Whilst not really ELF-specific, this is implemented only for ELF currently, so should be down in that section.

63 ↗(On Diff #212130)

This needs extra mark-up. See llvm-objcopy's documentation.

71 ↗(On Diff #212130)

This text isn't right. The switch has nothing to do with symbols. As with the other switches, use the text from llvm-objcopy.

81 ↗(On Diff #212130)

This should be grouped with --enable-deterministic-archives, like -D for --disable-deterministic archives.

85 ↗(On Diff #212130)

Also -V, right?

120 ↗(On Diff #212130)

Also -K?

llvm/tools/llvm-objcopy/StripOpts.td
35–36 ↗(On Diff #212130)

Do this in a separate change.

Thanks for working on this! You're missing a number of aliases from the list. Please make sure they are included in the doc.

Happy to help :). Will do.

Also, please use the text from the llvm-objcopy doc where the switch is common to the two tools.

Yes, good idea. In fact, I'm going to move the common switches into a new file, say CommonOpts.td, and include "CommonOpts.td" at the top of ObjcopyOpts.td and StripOpts.td. That way we don't have the same switch in two different places each with different HelpText.

mmpozulp updated this revision to Diff 213313.Aug 5 2019, 3:58 AM

Incorporate feedback from @jhenderson.

Move common help options from ObjcopyOpts.td and StripOpts.td
into CommonOpts.td and CommonAliases.td which are included
in the former files.

Add HelpText to aliases which did not already have it.

The tabelgen option changes should be in their own review. I haven't tried to review them here.

llvm/docs/CommandGuide/llvm-strip.rst
15 ↗(On Diff #213313)

Perhaps rephrase this to make the behaviour clearer. E.g. "If no other stripping or remove options are specified, --strip-all will be enabled by default."

87 ↗(On Diff #213313)

Change this to "remove all debug sections". Specifying other switches could mean that the "only" is invalid.

jhenderson added a subscriber: wolfgangp.

CC'ing @wolfgangp, due to D65787 adding a new switch. One of the two patches will need updating once the other lands.

mmpozulp updated this revision to Diff 214287.Aug 8 2019, 7:55 PM

Incorporate feedback from @jhenderson.
Add new switch --strip-sections. Thanks, James.

Also, I moved the tablegen changes to https://reviews.llvm.org/D65991

jhenderson accepted this revision.Aug 9 2019, 1:52 AM

LGTM! Thanks.

This revision is now accepted and ready to land.Aug 9 2019, 1:52 AM
This revision was automatically updated to reflect the committed changes.