Make cl::HideUnrelatedOptionsless error-prone

[serge-sans-paille]

Summary

The Sub optional parameter is never explicitly set. Quoting @hintonda

Only the special cl::SubCommand, cl::TopLevelSubCommand, which is the
default, can see all registered options, so passing it is the only way
to truly hide options not associated with the cl::Categorys of interest.
Any other combination of calls to cl::HideUnrelatedOptions with various
cl::SubCommands is unnecessary and error-prone.

[lebedev.ri]

Thanks! I do like the llvm-cat -help changes, and this does make sense in that context.
@hintonda can you comment on why the lhs of the diff is the way it is, why these were cl::ReallyHidden?

[hintonda]

This is a great idea, but I'd prefer it not unilaterally change current behavior.

llvm/lib/Support/CommandLine.cpp

2431

I think it would be better to default to the current behavior and add an additional default parameter to cl::HideUnrelatedOptions. That way, anyone using this options will continue to get the same behavior, while allowing others to pass cl::Hidden if they don't want them really hidden.

[hintonda]

In D61870#1500546, @lebedev.ri wrote:

Thanks! I do like the llvm-cat -help changes, and this does make sense in that context.
@hintonda can you comment on why the lhs of the diff is the way it is, why these were cl::ReallyHidden?

It was added in D7100, and I think the rational was to hide implementation details.

[hintonda]

In D61870#1500560, @hintonda wrote:

In D61870#1500546, @lebedev.ri wrote:

Thanks! I do like the llvm-cat -help changes, and this does make sense in that context.
@hintonda can you comment on why the lhs of the diff is the way it is, why these were cl::ReallyHidden?

It was added in D7100, and I think the rational was to hide implementation details.

Btw, here's a quick one liner for finding this stuff:

git log `git blame llvm/unittests/Support/CommandLineTest.cpp | grep " 383)" |cut -d ' ' -f 1`

[serge-sans-paille]

It was added in D7100, and I think the rational was to hide implementation details.

@hintonda: What about an optional `bool ReallyHide=true` paramer so that the same function could be use to really hide or just hide?

[hintonda]

In D61870#1500834, @serge-sans-paille wrote:

It was added in D7100, and I think the rational was to hide implementation details.

@hintonda: What about an optional `bool ReallyHide=true` paramer so that the same function could be use to really hide or just hide?

Yes, that's what I was trying to say, but since it's an enum, I'd rather you use the real type. Just set the default to ReallyHidden to maintain current behavior. Then you can just pass it to cl::setHiddenFlag() and be done with it.

[serge-sans-paille]

@hintonda this looks better that way, for sure.

[hintonda]

Just a few nits, otherwise looks pretty good.

llvm/include/llvm/Support/CommandLine.h

1992

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);

llvm/lib/Support/CommandLine.cpp

2426

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.

2427

Not sure this is needed. Why wouldn't you let someone remove a hidden flag? Might be useful in tests.

[hintonda]

llvm/include/llvm/Support/CommandLine.h

1992

I just grep'd all the projects in the repo, and I didn't find a single case of any calls to cl::HideUnrelatedOptions that passed SubCommand, so I think you could safely change the API to the new signature with OptionHidden as the second argument. That would simplify the API.

@beanz, how do you feel about that?

[serge-sans-paille]

@hintonda I've updated the patch taking your advices into account. I took a step ahead and applied the non-conservative signature change for cl::HideUnrelatedOptions

[lebedev.ri]

Looks good to me, but please wait for @hintonda / @beanz.

llvm/include/llvm/Support/CommandLine.h

1971

Comment needs updating

1982

Comment needs updating

[serge-sans-paille]

@lebedev.ri doc updated.

[hintonda]

Just a couple nits, otherwise looks great.

LGTM, but please give it a day for any further comments.

llvm/include/llvm/Support/CommandLine.h

1974

Could you document the 3rd parameter as well? Here and below.

1979

Please change the name of this parameter, here and elsewhere, to indicate it's a flag, not an Option.

[beanz]

This patch doesn't actually include how the new code will be used, and the review doesn't include enough context for me to understand what is going on.

Since this seems to be motivated by the llvm-cat option situation, which I believe is caused when using LLVM_LINK_LLVM_DYLIB, I expect that we will want to use cl::HideUnrelatedOptions in a lot more places to clean up other tool help messages. That makes me a little more concerned about the API and not having a clear picture of the use cases I'm not sure this is correct.

In particular I think the common case may be setting cl::ReallyHidden (I see no example otherwise), which means as soon as we start applying this tool to tools with subcommands the API gets messy.

[serge-sans-paille]

@beanz

I expect that we will want to use cl::HideUnrelatedOptions in a lot more places to clean up other tool help messages.

That's indeed the plan.

That makes me a little more concerned about the API and not having a clear picture of the use cases I'm not sure this is correct.
In particular I think the common case may be setting cl::ReallyHidden (I see no example otherwise), which means as soon as we start applying this tool to tools with subcommands the API gets messy.

As pointed out by @hintonda, current default argument is *never* explicitly used in the codebase. Concerning the tools, I plan to either set cl::ReallyHidden for simple tools like llvm-cat, or cl::Hidden for tools where extra options may matter but would clutter the -help output. llvm-as (see https://reviews.llvm.org/D60603) is one of those.

This may also mean that each option should have a category, but I'm unsure.

[beanz]

In D61870#1503490, @serge-sans-paille wrote:

@beanz
As pointed out by @hintonda, current default argument is *never* explicitly used in the codebase.

Yea, but that's going to change as soon as you start calling this in more places. Have you looked at how this might apply to a tool that has sub-commands? My concern is that the ordering of default parameters in functions that have more than one really needs to be in the order of decreasing likelihood of whether or not it will be overridden. By my count we have 2 tools with subcommands that account for 16 SubCommand, and you have one example of overriding the enum. At the moment it seems to me like overriding the SubCommand may be the more common case.

An alternative may be to provide two APIs, one that doesn't take a SubCommand, and one that does. The one that doesn't take a SubCommand could just call the API that does providing the default argument.

This may also mean that each option should have a category, but I'm unsure.

That probably isn't completely unreasonable. In fact there are some big benefits to that.

[hintonda]

In D61870#1503529, @beanz wrote:

In D61870#1503490, @serge-sans-paille wrote:

@beanz
As pointed out by @hintonda, current default argument is *never* explicitly used in the codebase.

Yea, but that's going to change as soon as you start calling this in more places. Have you looked at how this might apply to a tool that has sub-commands? My concern is that the ordering of default parameters in functions that have more than one really needs to be in the order of decreasing likelihood of whether or not it will be overridden. By my count we have 2 tools with subcommands that account for 16 SubCommand, and you have one example of overriding the enum. At the moment it seems to me like overriding the SubCommand may be the more common case.

Since no one is calling cl::HideUnrelatedOptions with specific SubCommands, it's hard to say. However, llvm-pdbutil makes extensive use of SubCommands, it seems like a good test-case.

The basic --help-hidden output includes a lot of additional general options, though I haven't investigated where they come from. However, the --help-hidden output for a specific SubCommand does not include those options, so I'm not sure this is a problem (I'll investigate exactly how it all works and report back). I'm temped to think SubCommands aren't really needed in this API, since only the Options for the SubCommand are printed anyway. Again, I'll investigate and report back.

But the biggest issue is that --help-hidden always displays all the Categories every time --help-hidden is passed, even if they are empty and wouldn't normally apply. I think that's a bug.

[hintonda]

In D61870#1503625, @hintonda wrote:

The basic --help-hidden output includes a lot of additional general options, though I haven't investigated where they come from. However, the --help-hidden output for a specific SubCommand does not include those options, so I'm not sure this is a problem (I'll investigate exactly how it all works and report back). I'm temped to think SubCommands aren't really needed in this API, since only the Options for the SubCommand are printed anyway. Again, I'll investigate and report back.

Sorry, what I meant to say was that when you print help for a SubCommand, you only get that SubCommand, so it doesn't seem to matter if the other ones are hidden or not. Of course you still need to specify when SubCommands you don't want hidden in the first place. In that case, SubCommand is probably the more important option as @beanz states, so it probably should come before OptionHidden. I'm open to additional API calls as well, but they should be driven by actual use-cases.

[serge-sans-paille]

@hintonda / @beanz : I've been thinking about this, and I'm now convinced that this review is probably not the good approach. What we need is a good way to track which options are actually important for a given binary, correctly set the categories of each option and *reallyhide* the others. So I'll pause this review and start working on sanitizing the output of several tools until I get a better understanding of the actual API needs.

[hintonda]

In D61870#1506692, @serge-sans-paille wrote:

@hintonda / @beanz : I've been thinking about this, and I'm now convinced that this review is probably not the good approach. What we need is a good way to track which options are actually important for a given binary, correctly set the categories of each option and *reallyhide* the others. So I'll pause this review and start working on sanitizing the output of several tools until I get a better understanding of the actual API needs.

I created an example application yesterday to test this stuff out, and was just about to add a similar comment.

The problem with this API -- not your changes, but the API in general -- is that the HiddenFlag attribute is per cl::Option, not per cl::SubCommand. So, if you hide an option, it's hidden everywhere. However, finding the options you want to hide can be tricky. Only the special cl::SubCommand, cl::TopLevelSubCommand, which is the default, can see all registered options, so passing it is the only way to truly hide options not associated with the cl::Categorys of interest. Any other combination of calls to cl::HideUnrelatedOptions with various cl::SubCommands is unnecessary and error-prone.

Therefore, I recommend removing the SubCommand parameter from this API and always using cl::TopLevelSubCommand.

[hintonda]

In D61870#1506758, @hintonda wrote:

In D61870#1506692, @serge-sans-paille wrote:

@hintonda / @beanz : I've been thinking about this, and I'm now convinced that this review is probably not the good approach. What we need is a good way to track which options are actually important for a given binary, correctly set the categories of each option and *reallyhide* the others. So I'll pause this review and start working on sanitizing the output of several tools until I get a better understanding of the actual API needs.

I created an example application yesterday to test this stuff out, and was just about to add a similar comment.

The problem with this API -- not your changes, but the API in general -- is that the HiddenFlag attribute is per cl::Option, not per cl::SubCommand. So, if you hide an option, it's hidden everywhere. However, finding the options you want to hide can be tricky. Only the special cl::SubCommand, cl::TopLevelSubCommand, which is the default, can see all registered options, so passing it is the only way to truly hide options not associated with the cl::Categorys of interest. Any other combination of calls to cl::HideUnrelatedOptions with various cl::SubCommands is unnecessary and error-prone.

Therefore, I recommend removing the SubCommand parameter from this API and always using cl::TopLevelSubCommand.

Actually, this isn't quit true, TopLevelSubCommand doesn't keep a recored of all options, however, we still iterate over all SubCommands to have the same affect.

[serge-sans-paille]

Therefore, I recommend removing the SubCommand parameter from this API and always using cl::TopLevelSubCommand.

Let's go that way for this review, and don't change anything about the Hidden/ReallyHidden status.

[hintonda]

Btw, TopLevelSubCommand can't actually see every option, just those not specifically assigned to another SubCommand. Typically, that only happens in applications, so this should work okay. However, it would probably be safer to iterate over all SubCommands instead of just looking TopLevelSubCommand. Though I'm not sure that really matters.

While I can't conceive of any reason I'd want to pass a SubCommand, it would be better to come up with a motivating example of how you plan to use this before we make any changes, since specific use-cases might require a slightly different implementation.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

llvm/include/llvm/Support/CommandLine.h

 /// with nullptrs in the Argv vector.
 /// \param [in] RelativeNames true if names of nested response files must be
 /// resolved relative to including file.
 /// \return true if all @files were expanded successfully or there were none.
 bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
                          SmallVectorImpl<const char *> &Argv,
                          bool MarkEOLs = false, bool RelativeNames = false);
 
 /// Mark all options not part of this category as cl::ReallyHidden.
 ///
 /// \param Category the category of options to keep displaying
 ///
 /// Some tools (like clang-format) like to be able to hide all options that are
 /// not specific to the tool. This function allows a tool to specify a single
 /// option category to display in the -help output.
 void HideUnrelatedOptions(cl::OptionCategory &Category,
+                          cl::OptionHidden Opt = cl::ReallyHidden,
                           SubCommand &Sub = *TopLevelSubCommand);
 
 /// Mark all options not part of the categories as cl::ReallyHidden.
 ///
 /// \param Categories the categories of options to keep displaying.
 ///
 /// Some tools (like clang-format) like to be able to hide all options that are
 /// not specific to the tool. This function allows a tool to specify a single
 /// option category to display in the -help output.
 void HideUnrelatedOptions(ArrayRef<const cl::OptionCategory *> Categories,
+                          OptionHidden Opt = cl::ReallyHidden,
                           SubCommand &Sub = *TopLevelSubCommand);
 
 /// Reset all command line options to a state that looks as if they have
 /// never appeared on the command line.  This is useful for being able to parse
 /// a command line multiple times (especially useful for writing tests).
 void ResetAllOptionOccurrences();
 
 /// Reset the command line parser back to its initial state.  This
 /// removes
 /// all options, categories, and subcommands and returns the parser to a state
 /// where no options are supported.
 void ResetCommandLineParser();
 
 } // end namespace cl
 
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_COMMANDLINE_H

llvm/lib/Support/CommandLine.cpp

StringMap<Option *> &cl::getRegisteredOptions(SubCommand &Sub) {
   return Sub.OptionsMap;
 }
 
 iterator_range<typename SmallPtrSet<SubCommand *, 4>::iterator>
 cl::getRegisteredSubcommands() {
   return GlobalParser->getRegisteredSubcommands();
 }
 
-void cl::HideUnrelatedOptions(cl::OptionCategory &Category, SubCommand &Sub) {
+void cl::HideUnrelatedOptions(cl::OptionCategory &Category, OptionHidden Opt,
+                              SubCommand &Sub) {
   for (auto &I : Sub.OptionsMap) {
     for (auto &Cat : I.second->Categories) {
       if (Cat != &Category &&
           Cat != &GenericCategory)
-        I.second->setHiddenFlag(cl::ReallyHidden);
+        I.second->setHiddenFlag(Opt);
     }
   }
 }
 
 void cl::HideUnrelatedOptions(ArrayRef<const cl::OptionCategory *> Categories,
-                              SubCommand &Sub) {
+                              OptionHidden Opt, SubCommand &Sub) {
   for (auto &I : Sub.OptionsMap) {
     for (auto &Cat : I.second->Categories) {
       if (find(Categories, Cat) == Categories.end() && Cat != &GenericCategory)
-        I.second->setHiddenFlag(cl::ReallyHidden);
+        I.second->setHiddenFlag(Opt);
     }
   }
 }
 
 void cl::ResetCommandLineParser() { GlobalParser->reset(); }
 void cl::ResetAllOptionOccurrences() {
   GlobalParser->ResetAllOptionOccurrences();
 }
 
 void LLVMParseCommandLineOptions(int argc, const char *const *argv,
                                  const char *Overview) {
   llvm::cl::ParseCommandLineOptions(argc, argv, StringRef(Overview),
                                     &llvm::nulls());
 }

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, AliasRequired) {
   testAliasRequired(array_lengthof(opts1), opts1);
   testAliasRequired(array_lengthof(opts2), opts2);
 }
 
 TEST(CommandLineTest, HideUnrelatedOptions) {
   StackOption<int> TestOption1("hide-option-1");
   StackOption<int> TestOption2("hide-option-2", cl::cat(TestCategory));
 
+  cl::HideUnrelatedOptions(TestCategory, cl::Hidden);
+
+  ASSERT_EQ(cl::Hidden, TestOption1.getOptionHiddenFlag())
+      << "Failed to hide extra option.";
+  ASSERT_EQ(cl::NotHidden, TestOption2.getOptionHiddenFlag())
+      << "Hid extra option that should be visible.";
+
+  StringMap<cl::Option *> &Map =
+      cl::getRegisteredOptions(*cl::TopLevelSubCommand);
+  ASSERT_EQ(cl::NotHidden, Map["help"]->getOptionHiddenFlag())
+      << "Hid default option that should be visible.";
+}
+
+TEST(CommandLineTest, ReallyHideUnrelatedOptions) {
+  StackOption<int> TestOption1("hide-option-1");
+  StackOption<int> TestOption2("hide-option-2", cl::cat(TestCategory));
+
   cl::HideUnrelatedOptions(TestCategory);
 
   ASSERT_EQ(cl::ReallyHidden, TestOption1.getOptionHiddenFlag())
       << "Failed to hide extra option.";
   ASSERT_EQ(cl::NotHidden, TestOption2.getOptionHiddenFlag())
-      << "Hid extra option that should be visable.";
+      << "Hid extra option that should be visible.";
 
   StringMap<cl::Option *> &Map =
       cl::getRegisteredOptions(*cl::TopLevelSubCommand);
   ASSERT_EQ(cl::NotHidden, Map["help"]->getOptionHiddenFlag())
-      << "Hid default option that should be visable.";
+      << "Hid default option that should be visible.";
 }
 
 cl::OptionCategory TestCategory2("Test Options set 2", "Description");
 
 TEST(CommandLineTest, HideUnrelatedOptionsMulti) {
   StackOption<int> TestOption1("multi-hide-option-1");
   StackOption<int> TestOption2("multi-hide-option-2", cl::cat(TestCategory));
   StackOption<int> TestOption3("multi-hide-option-3", cl::cat(TestCategory2));
 
   const cl::OptionCategory *VisibleCategories[] = {&TestCategory,
                                                    &TestCategory2};
 
+  cl::HideUnrelatedOptions(makeArrayRef(VisibleCategories), cl::Hidden);
+
+  ASSERT_EQ(cl::Hidden, TestOption1.getOptionHiddenFlag())
+      << "Failed to hide extra option.";
+  ASSERT_EQ(cl::NotHidden, TestOption2.getOptionHiddenFlag())
+      << "Hid extra option that should be visible.";
+  ASSERT_EQ(cl::NotHidden, TestOption3.getOptionHiddenFlag())
+      << "Hid extra option that should be visible.";
+
+  StringMap<cl::Option *> &Map =
+      cl::getRegisteredOptions(*cl::TopLevelSubCommand);
+  ASSERT_EQ(cl::NotHidden, Map["help"]->getOptionHiddenFlag())
+      << "Hid default option that should be visible.";
+}
+
+TEST(CommandLineTest, ReallyHideUnrelatedOptionsMulti) {
+  StackOption<int> TestOption1("multi-hide-option-1");
+  StackOption<int> TestOption2("multi-hide-option-2", cl::cat(TestCategory));
+  StackOption<int> TestOption3("multi-hide-option-3", cl::cat(TestCategory2));
+
+  const cl::OptionCategory *VisibleCategories[] = {&TestCategory,
+                                                   &TestCategory2};
+
   cl::HideUnrelatedOptions(makeArrayRef(VisibleCategories));
 
   ASSERT_EQ(cl::ReallyHidden, TestOption1.getOptionHiddenFlag())
       << "Failed to hide extra option.";
   ASSERT_EQ(cl::NotHidden, TestOption2.getOptionHiddenFlag())
-      << "Hid extra option that should be visable.";
+      << "Hid extra option that should be visible.";
   ASSERT_EQ(cl::NotHidden, TestOption3.getOptionHiddenFlag())
-      << "Hid extra option that should be visable.";
+      << "Hid extra option that should be visible.";
 
   StringMap<cl::Option *> &Map =
       cl::getRegisteredOptions(*cl::TopLevelSubCommand);
   ASSERT_EQ(cl::NotHidden, Map["help"]->getOptionHiddenFlag())
-      << "Hid default option that should be visable.";
+      << "Hid default option that should be visible.";
 }
 
 TEST(CommandLineTest, SetValueInSubcategories) {
   cl::ResetCommandLineParser();
 
   StackSubCommand SC1("sc1", "First subcommand");
   StackSubCommand SC2("sc2", "Second subcommand");