[PATCH] D65384: [Docs][llvm-strip] Add help text to llvm-strip rst doc

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:2
+llvm-strip - object stripping tool
+==============================================
+
----------------
Delete the trailing '=' here.


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:9
+
+:program:`llvm-strip` [*options*] *input* [*output*]
+
----------------
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). 


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:14
+
+:program:`llvm-strip` is a tool to strip sections and symbols from object files.
+
----------------
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).


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:27
+                         
+  Disable deterministic mode when stripping archives (use real values for UIDs, GIDs, and timestamps).
+
----------------
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.


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:33-35
+.. option::  --discard-locals
+
+  Remove compiler-generated local symbols, (e.g. symbols starting with .L).
----------------
This should be in the "ELF-specific" section.


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:40
+  Enable deterministic mode when stripping archives (use zero for UIDs, GIDs, and timestamps).
+
+.. option::  --no-strip-all
----------------
What about -h/--help?


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:47
+
+  Write output to <file>.
+
----------------
This should say something about what happens if there are multiple input files.


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:49-51
+.. option::  --preserve-dates
+
+  Preserve access and modification timestamps.
----------------
Whilst not really ELF-specific, this is implemented only for ELF currently, so should be down in that section.


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:63
+
+  Compatible with GNU strip's --strip-all.
+
----------------
This needs extra mark-up. See llvm-objcopy's documentation.


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:71
+
+  Remove debugging symbols only.
+
----------------
This text isn't right. The switch has nothing to do with symbols. As with the other switches, use the text from llvm-objcopy.


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:81
+
+.. option::  -U
+
----------------
This should be grouped with --enable-deterministic-archives, like -D for --disable-deterministic archives.


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:85
+
+.. option::  --version
+
----------------
Also -V, right?


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:120
+
+.. option::  --keep-symbol=symbol
+
----------------
Also -K?


================
Comment at: llvm/tools/llvm-objcopy/StripOpts.td:35-36
 
-def output : JoinedOrSeparate<["-"], "o">, HelpText<"Write output to <file>">;
+def output : JoinedOrSeparate<["-"], "o">, HelpText<"Write output to <file>">,
+             MetaVarName<"file">;
 
----------------
Do this in a separate change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65384/new/

https://reviews.llvm.org/D65384