Page MenuHomePhabricator

[cl] Teach cl to support the notion of sub commands (e.g. "git checkout <foo>")
ClosedPublic

Authored by zturner on Jun 17 2016, 4:28 PM.

Details

Summary

This patch improves cl to support sub commands. For example, "git checkout", "git status", etc where each subcommand has an unrelated set of options.

An example of how to use this new functionality is included in this patch where the llvm-pdbdump tool is updated to use this new functionality. If / when this patch is accepted, I will only submit with the changes to cl, and then submit the changes to llvm-pdbdump as a followup. It's only included here as an example.

Any code which was previously written against cl will continue to work unmodified. If you don't specify a subcommand for an option, commands go into the "top level" subcommand, which is searched when no subcommand is specified.

There is another special subcommand called AllSubcommands which, when specified, will inject an option into every sub command. This is useful, for example, to make foo.exe --help and foo.exe command --help both work and do the right thing.

options can also have multiple subcommands, so that the same option will appear in multiple subcommands.

Help output is updated as well to print info about supported subcommands, and also to print the list of options for individual subcommands.

Not sure who to include as a reviewer, feel free to add others as appropriate.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 61142.Jun 17 2016, 4:28 PM
zturner retitled this revision from to [cl] Teach cl to support the notion of sub commands (e.g. "git checkout <foo>").
zturner updated this object.
zturner added reviewers: beanz, chandlerc, rnk.
zturner added a subscriber: llvm-commits.
zturner updated this revision to Diff 61148.Jun 17 2016, 5:29 PM

Make the usage in llvm-pdbdump a little cleaner and separate the responsibilities of the different sub-commands a little better.

zturner updated this revision to Diff 61158.Jun 17 2016, 9:11 PM

Added some unit tests. It wasn't easy to test this because the normal paradigm for using llvm::cl is using globals, which doesn't allow you to set up a clean test case each time through. So I had to go through and add some hooks in llvm::cl for basically wiping the slate clean on each test. This allows me to set up new subcommands and options on the stack for each test.

The good news is, adding these unit tests caught a couple of bugs in the implementation of AllSubCommands, which are now fixed.

zturner updated this revision to Diff 61159.Jun 17 2016, 10:38 PM

Fixed one last set of issues causing some tests to fail. I may work on adding some more unit tests if I come up with good things to test, but for now I think this is complete functionality and bugfix wise.

Yes, i was not aware of that. Will do on Monday

ruiu added a subscriber: ruiu.Jun 19 2016, 10:57 PM

Can you split this patch into two patches -- one is to add the subcommand system and the other is to use it in llvm-pdbdump. Since the feature itself is pretty generic, I don't think it is a good idea to mix it with other change.

I mentioned in the description that I would eventually submit the two
separately, I included it here to show an example of the syntax to use the
new feature. If you prefer i can remove it entirely from this review

amccarth added inline comments.
lib/Support/CommandLine.cpp
141 ↗(On Diff #61159)

addOption seems very similar to addLiteralOption. Is it possible to reduce the code duplication by having addOption just forward to addLiteralOption with O->ArgStr as the Name argument? Am I missing some subtle difference?

192 ↗(On Diff #61159)

It appears this method should never be called with AllSubCommands. Is that true? If not, can you document that and maybe have the code check to prevent it.

ruiu added a comment.Jun 20 2016, 4:39 PM

If it takes time, you don't need to do it, but it is easier to review if it were split into two because I don't need to find out which diffs are "real" parts and which are "examples".

zturner updated this revision to Diff 61401.Jun 21 2016, 10:21 AM

This reverts the llvm-pdbdump part of the changes so that the diff is much smaller. Anyone interested in seeing that can just look at an earlier revision of this diff.

rengolin added a reviewer: bogner.
rengolin added subscribers: rengolin, probinson.
beanz added inline comments.Jun 21 2016, 5:00 PM
lib/Support/CommandLine.cpp
141 ↗(On Diff #61401)

Yes, addOption could call addLiteralOption and reduce duplication. The methods do serve different purposes, so they both need to be present. The, poorly named (by me), addLiteralOption adds the option's string and cl::Option to the global option mapping.

192 ↗(On Diff #61401)

It needs to work if it is used with AllSubCommands, and TopLevelSubCommand. If it doesn't that will regress functionality.

ruiu added inline comments.Jun 22 2016, 1:21 AM
include/llvm/Support/CommandLine.h
207 ↗(On Diff #61401)

You can write = nullptr here.

lib/Support/CommandLine.cpp
113 ↗(On Diff #61401)

Early return.

1784–1785 ↗(On Diff #61401)

You can merge the two if conditions.

unittests/Support/CommandLineTest.cpp
303–304 ↗(On Diff #61401)

Why don't you use EXPECT_FALSE and EXPECT_TRUE?

341 ↗(On Diff #61401)

Remove.

zturner updated this revision to Diff 62006.Jun 27 2016, 1:00 PM

Sorry for the delay, I was out the second half of last week. This takes all the previous suggestions into account, and adds unit tests to make sure that removing from AllSubCommands and TopLevelSubCommand works as expected.

@beanz, are you ok with this change now given that there is a unit test
verifying that removeOption works with AllSubCommands and
TopLevelSubCommand?

beanz accepted this revision.Jun 28 2016, 12:49 PM
beanz edited edge metadata.

Yep. LGTM!

Thanks for doing this, I think it will be very useful.

-Chris

This revision is now accepted and ready to land.Jun 28 2016, 12:49 PM
This revision was automatically updated to reflect the committed changes.