[PATCH] D104363: [llvm] Mark more internal command line optins as cl::Hidden

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Some test changes aren't correct, so requesting changes.



================
Comment at: llvm/test/tools/llvm-strings/help.test:11-14
 CATEG:    Generic Options:
+LIST-NOT: General options:
+CATEG:    Tool Options:
 LIST-NOT: Generic Options:
----------------
This isn't a correct change.

Before, the intent was that the `LIST-NOT` checks showed that each of the checked-for categories appeared in `--help` output, but not `--help-hidden`.

The checks should be:
```
CATEG:     Generic Options:
LIST-NOT: Generic Options:
CATEG:     Tool Options:
LIST-NOT: Tool Options:
```

Additionally, you can add `--implicit-check-not="General options:"` to the `--help` FileCheck, to show that the "unrelated" options are not present in the output.

Finally, I don't know if it's still present, but in the llvm-strings copy I have installed on my machine, there's a "Color Options:" category that is also listed in the help text. It should probably be tested here too.


================
Comment at: llvm/tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp:39
 
-static cl::opt<std::string>
-    InputFilename(cl::Positional, cl::desc("<input bitcode>"), cl::init("-"));
+static cl::OptionCategory ToolOptions("Tool Options");
+
----------------
In other tools, we either do, or used to at least, call the tool-specific option category after the tool name. Here, that would mean something like `BCAnalyzerOptions("BC Analyzer Options")`.

It may also be a good idea to add testing similar to the llvm-strings test to show that only the expected options categories are present.

The same comments apply to each of the other tools.


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:65
 static cl::list<std::string>
 Decorated(cl::Positional, cl::desc("<mangled>"), cl::ZeroOrMore);
 
----------------
In other cases, you've added the option category to positionals as well. I don't know if you need to, but we should be consistent either way, and either remove it from all the others, or add it here.


================
Comment at: llvm/tools/llvm-lto/llvm-lto.cpp:111
 
 cl::opt<ThinLTOModes> ThinLTOMode(
     "thinlto-action", cl::desc("Perform a single ThinLTO stage:"),
----------------
There are a load more options here that seem like they syhould have a category?


================
Comment at: llvm/tools/llvm-xray/llvm-xray.cpp:25
 
+static cl::OptionCategory ToolOptions("Tool Options");
+
----------------
Seems like you don't need the option category here at all?


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

https://reviews.llvm.org/D104363