[PATCH] D104363: [llvm] [tools] Hide unrelated options from all tools

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 6 00:30:34 PDT 2021


jhenderson added a comment.

Where you've added the `ColorCategory`, how sure are you that it actually has an impact on the tool you've modified. My suspicion is that in many cases it actually has no effect whatsoever, as the tool in question never uses the `WithColor` code. Please make sure you add it only where the option has some visible effect.



================
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:
----------------
jhenderson wrote:
> 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.
This still isn't correct for the code this patch is based on.

That being said, @MaskRay has just landed a patch to completely redo llvm-strings command-line handling, so it's moot - please rebase.


================
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");
+
----------------
jhenderson wrote:
> 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.
> 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.

It seems like this part of my comment has been missed?


================
Comment at: llvm/tools/llvm-extract/llvm-extract.cpp:143
   LLVMContext Context;
-  cl::HideUnrelatedOptions(ExtractCat);
+  cl::HideUnrelatedOptions({&ExtractCat, &llvm::ColorCategory});
   cl::ParseCommandLineOptions(argc, argv, "llvm extractor\n");
----------------
Seems like this is only tangentially related to this patch. Please move it to another patch. Also check to see if --color actually should appear in the otuput - if it has no impact, it shouldn't.


================
Comment at: llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp:472
+  HideUnrelatedOptions({&GeneralOptions, &ConversionOptions, &LookupOptions,
+                        &llvm::ColorCategory});
   cl::ParseCommandLineOptions(argc, argv, Overview);
----------------
Same as above. Don't add this in this patch.


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:454
   // Parse arguments.
+  cl::HideUnrelatedOptions({&IfsOptions, &llvm::ColorCategory});
   cl::ParseCommandLineOptions(argc, argv);
----------------
Here and in many other cases, we're already `using namespace llvm`, so you shouldn't need the `llvm` prefix before `ColorCategory`.


================
Comment at: llvm/tools/llvm-lto/llvm-lto.cpp:67
 
+static cl::OptionCategory LtoOptions("LTO Options");
 static cl::opt<char>
----------------
LTO is an abbreviation, so should be capitalized in the variable name too (i.e. `LTOOptions`).


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

https://reviews.llvm.org/D104363



More information about the llvm-commits mailing list