[PATCH] D105532: [llvm-readobj] Switch command line parsing from llvm::cl to OptTable

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-readobj/Opts.td:92
+def : F<"W", "Ignored for GNU readelf compatibility">;
+def : FF<"wide", "">, Flags<[HelpHidden]>;
+
----------------
Any particular reason --wide is hidden, but -W isn't? Both are in the help text of GNU readelf. I don't really mind which way you jump here. I just think they should both be the same (hidden or visible). If push comes to shove, I'd leave them as visible.


================
Comment at: llvm/tools/llvm-readobj/Opts.td:103
+def : FF<"dynamic", "Alias for --dynamic-table">, Alias<dynamic_table>;
+def : FF<"elf-cg-profile", "Alias for --cg-profile">, Alias<cg_profile>, Flags<[HelpHidden]>;
+def : FF<"elf-hash-histogram", "Alias for --histogram">, Alias<histogram>, Flags<[HelpHidden]>;
----------------
These elf-* options are all in the docs and should be removed if they are being marked as hidden.


================
Comment at: llvm/tools/llvm-readobj/Opts.td:107
+def : FF<"file-headers", "Alias for --file-header">, Alias<file_header>, Flags<[HelpHidden]>;
+def : FF<"relocations", "Alias for --relocs">, Alias<relocs>, Flags<[HelpHidden]>;
+def : FF<"sections", "Alias for --section-headers">, Alias<section_headers>;
----------------
I'm not sure about hiding this one. If you really feel it should be hidden, remove it from both docs (currently it's only missing from one).


================
Comment at: llvm/tools/llvm-readobj/Opts.td:48
+// ELF specific options.
+def grp_elf : OptionGroup<"kind">, HelpText<"llvm-readobj ELF Specific Options">;
+def dynamic_table : FF<"dynamic-table", "Display the dynamic section table">, Group<grp_elf>;
----------------
MaskRay wrote:
> MaskRay wrote:
> > jhenderson wrote:
> > > We should probably drop the "llvm-readobj" bit from these descriptions, as the tool is usually installed as "llvm-readelf" on people's systems, so the name would be incorrect there. Here, we can simply say "ELF Specific Options".
> > The option groups are sorted alphabetically, so "ELF Specific Options" would be before "OPTIONS"... That's how the `llvm-readobj` prefix helps,  `OPTIONS` < `llvm-readobj ...`
> I am going to use the `OPTIONS (ELF specific)` style for now.
Thanks, I was going to suggest something along the same lines, but you beat me to it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105532