Page MenuHomePhabricator

Make cl::HideUnrelatedOptionsless error-prone
Needs RevisionPublic

Authored by serge-sans-paille on May 13 2019, 1:19 PM.

Details

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.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2019, 1:19 PM

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 requested changes to this revision.May 13 2019, 2:09 PM

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

llvm/lib/Support/CommandLine.cpp
2430

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.

This revision now requires changes to proceed.May 13 2019, 2:09 PM

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.

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`

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?

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 retitled this revision from Have cl::HideUnrelatedOptions just Hide and not ReallyHide options to Make cl::HideUnrelatedOptions more flexible.
serge-sans-paille edited the summary of this revision. (Show Details)

@hintonda this looks better that way, for sure.

Just a few nits, otherwise looks pretty good.

llvm/include/llvm/Support/CommandLine.h
1991

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
2425

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.

2426

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

hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
1991

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 edited the summary of this revision. (Show Details)

@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 accepted this revision.May 15 2019, 5:17 AM

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

llvm/include/llvm/Support/CommandLine.h
1971

Comment needs updating

1982

Comment needs updating

hintonda accepted this revision.May 15 2019, 9:09 AM

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.

1980

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

This revision is now accepted and ready to land.May 15 2019, 9:09 AM

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.

@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 added a comment.May 15 2019, 1:08 PM

@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.

@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.

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.

@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 / @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 / @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.

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.

serge-sans-paille retitled this revision from Make cl::HideUnrelatedOptions more flexible to Make cl::HideUnrelatedOptionsless error-prone.
serge-sans-paille edited the summary of this revision. (Show Details)
hintonda requested changes to this revision.May 17 2019, 2:29 PM

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.

This revision now requires changes to proceed.May 17 2019, 2:29 PM