Differential D61870
Make cl::HideUnrelatedOptionsless error-prone Authored by serge-sans-paille on May 13 2019, 1:19 PM.
Details
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.
Diff Detail Event TimelineComment Actions Thanks! I do like the llvm-cat -help changes, and this does make sense in that context. Comment Actions This is a great idea, but I'd prefer it not unilaterally change current behavior.
Comment Actions 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` Comment Actions 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. Comment Actions Just a few nits, otherwise looks pretty good.
Comment Actions @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 Comment Actions Just a couple nits, otherwise looks great. LGTM, but please give it a day for any further comments.
Comment Actions 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. Comment Actions
That's indeed the plan.
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. Comment Actions 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.
That probably isn't completely unreasonable. In fact there are some big benefits to that. Comment Actions 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. Comment Actions 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. Comment Actions @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. Comment Actions 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. Comment Actions 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. Comment Actions
Let's go that way for this review, and don't change anything about the Hidden/ReallyHidden status. Comment Actions 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. | ||||||||||||||||||||||||||||||||||||||||
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.