[PATCH] D61870: Make cl::HideUnrelatedOptions more flexible

[Don Hinton via Phabricator via llvm-commits]

hintonda added a comment.

Just a few nits, otherwise looks pretty good.



================
Comment at: llvm/include/llvm/Support/CommandLine.h:1994
+                          OptionHidden Opt = cl::ReallyHidden);
+void HideUnrelatedOptions(ArrayRef<const cl::OptionCategory *> Categories,
+                          OptionHidden Opt,
----------------
Not sure how I feel about the new signatures.  

If someone wants to call this and pass something other than `cl::ReallyHidden`, how difficult is it for them to use the original call with the extra parameter?  e.g.:

```
cl::HideUnrelatedOptions(SomeCat, *cl::TopLevelSubCommand, cl::Hidden);
```


================
Comment at: llvm/lib/Support/CommandLine.cpp:2426
+void cl::HideUnrelatedOptions(cl::OptionCategory &Category, SubCommand &Sub,
+                              OptionHidden Opt) {
+  assert(Opt != NotHidden && "Hiding something");
----------------
Since this is a flag, please change the name, here and elsewhere, to something like `HiddenFlag` so readers won't confuse it with an Option, which often appears in the file as `Opt`.


================
Comment at: llvm/lib/Support/CommandLine.cpp:2427
+                              OptionHidden Opt) {
+  assert(Opt != NotHidden && "Hiding something");
   for (auto &I : Sub.OptionsMap) {
----------------
Not sure this is needed.  Why wouldn't you let someone remove a hidden flag?  Might be useful in tests.


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

https://reviews.llvm.org/D61870