[PATCH] D104889: [llvm-strings] Switch command line parsing from llvm::cl to OptTable

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: gbreynoo.
jhenderson added a comment.

> one-dash long options like -all are supported

"are no longer"?

Generally seems reasonable to me. However, I am currently assigned to work outside our downstream binutils usage, so don't have time to investigate any ramifications. @gbreynoo, could you please check our downstream internal code bases and see if any of the behaviour changes @MaskRay mentions are going to impact them? If not, then it's okay by me. You'll want an internal ticket to track this change too, since it will require a downstream doc update.

The CommandGuide mentions --help-list, so will need updating as part of this change.



================
Comment at: llvm/tools/llvm-strings/Opts.td:18
+def print_file_name : Flag<["--"], "print-file-name">, HelpText<"Print the name of the file before each string">;
+defm radix : Eq<"radix", "Print the offset within the file">;
+def version : Flag<["--"], "version">, HelpText<"Display the version">;
----------------
This should probably list the valid values.


================
Comment at: llvm/tools/llvm-strings/Opts.td:22
+def : Flag<["-"], "a">, Alias<all>, HelpText<"Alias for --all">;
+def : Flag<["-"], "f">, Alias<print_file_name>, HelpText<"Alias for --print_file_name">;
+def : Flag<["-"], "h">, Alias<help>;
----------------



================
Comment at: llvm/tools/llvm-strings/llvm-strings.cpp:153
+
+  AllSections = Args.hasArg(OPT_all);
+  parseIntArg(Args, OPT_bytes_EQ, MinLength);
----------------
If the command guide is correct, I think you can drop this assignment and the corresponding variable (but not the option). The command guide says: `Silently ignored. Present for GNU strings compatibility.` Also, update the help text accordingly, I'd recommend.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104889