[llvm] r274171 - Resubmit "Update llvm command line parser to support subcommands."
Alex L via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 10 10:21:27 PDT 2016
On 10 October 2016 at 17:07, Zachary Turner <zturner at google.com> wrote:
> If you can post the lines of code that define your option set, maybe I can
> see if anything stands out as obvious.
>
It's actually a bug it seems, I can reproduce it in llvm-pdbdump as well if
I use only 2 subcommands there. A patch like this should reproduce the
problem:
diff --git a/tools/llvm-pdbdump/llvm-pdbdump.cpp
b/tools/llvm-pdbdump/llvm-pdbdump.cpp
index a6dd560..6ead97d 100644
--- a/tools/llvm-pdbdump/llvm-pdbdump.cpp
+++ b/tools/llvm-pdbdump/llvm-pdbdump.cpp
@@ -78,12 +78,8 @@ cl::SubCommand RawSubcommand("raw", "Dump raw structure
of the PDB file");
cl::SubCommand
PrettySubcommand("pretty",
"Dump semantic information about types and symbols");
-cl::SubCommand
- YamlToPdbSubcommand("yaml2pdb",
- "Generate a PDB file from a YAML description");
-cl::SubCommand
- PdbToYamlSubcommand("pdb2yaml",
- "Generate a detailed YAML description of a PDB
File");
+cl::SubCommand &YamlToPdbSubcommand = RawSubcommand;
+cl::SubCommand &PdbToYamlSubcommand = RawSubcommand;
cl::OptionCategory TypeCategory("Symbol Type Options");
cl::OptionCategory FilterCategory("Filtering Options");
>
> On Mon, Oct 10, 2016 at 9:03 AM Alex L <arphaman at gmail.com> wrote:
>
>> On 10 October 2016 at 16:56, Zachary Turner <zturner at google.com> wrote:
>>
>> So here, `Sub` refers to the active SubCommand. For example, if you
>> typed "llvm-pdbdump raw <options>" then `Sub` will refer to the
>> RawSubcommand. There are two special subcommands, `TopLevelSubcommand` and
>> `AllSubcommands`. The former becomes the active subcommand when you type
>> no options at all. For example, if you were to run `llvm-pdbdump -foo"
>> Then the active subcommand would be `TopLevelSubcommand`. Since
>> subcommands cannot be nested, `TopLevelSubcommand` is the only subcommand
>> from which it is possible for other subcommands to be "children" of. So
>> the first part of the conditional is saying "Only print this if the user
>> did not explicitly specify any subcommand". The >2 comes from the fact
>> that `TopLevelSubcommand` and `AllSubcommands` are builtin subcommands.
>> Even if someone does not define any subcommands for their tool, there will
>> still be those two. So the second check is equivalent to saying "If the
>> user has explicitly defined at least 1 subcommand".
>>
>>
>> Thanks for the explanation, that makes sense. I stumbled across this when
>> I was investigating why the help didn't print out subcommands when I had
>> less than 3 sub-commands in a private tool. Given this explanation it has
>> to be some mistake in my code or my reasoning. I'll double-check my code.
>>
>> Alex
>>
>>
>>
>> On Mon, Oct 10, 2016 at 3:44 AM Alex L <arphaman at gmail.com> wrote:
>>
>> On 29 June 2016 at 22:48, Zachary Turner via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>> Author: zturner
>> Date: Wed Jun 29 16:48:26 2016
>> New Revision: 274171
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=274171&view=rev
>> Log:
>> Resubmit "Update llvm command line parser to support subcommands."
>>
>> ....
>>
>> //===------------------------------------------------------
>> ----------------===//
>> @@ -1460,6 +1672,11 @@ static int OptNameCompare(const std::pai
>> return strcmp(LHS->first, RHS->first);
>> }
>>
>> +static int SubNameCompare(const std::pair<const char *, SubCommand *>
>> *LHS,
>> + const std::pair<const char *, SubCommand *>
>> *RHS) {
>> + return strcmp(LHS->first, RHS->first);
>> +}
>> +
>> // Copy Options into a vector so we can sort them as we like.
>> static void sortOpts(StringMap<Option *> &OptMap,
>> SmallVectorImpl<std::pair<const char *, Option *>>
>> &Opts,
>> @@ -1488,6 +1705,17 @@ static void sortOpts(StringMap<Option *>
>> array_pod_sort(Opts.begin(), Opts.end(), OptNameCompare);
>> }
>>
>> +static void
>> +sortSubCommands(const SmallPtrSetImpl<SubCommand *> &SubMap,
>> + SmallVectorImpl<std::pair<const char *, SubCommand *>>
>> &Subs) {
>> + for (const auto &S : SubMap) {
>> + if (S->getName() == nullptr)
>> + continue;
>> + Subs.push_back(std::make_pair(S->getName(), S));
>> + }
>> + array_pod_sort(Subs.begin(), Subs.end(), SubNameCompare);
>> +}
>> +
>> namespace {
>>
>> class HelpPrinter {
>> @@ -1495,12 +1723,25 @@ protected:
>> const bool ShowHidden;
>> typedef SmallVector<std::pair<const char *, Option *>, 128>
>> StrOptionPairVector;
>> + typedef SmallVector<std::pair<const char *, SubCommand *>, 128>
>> + StrSubCommandPairVector;
>> // Print the options. Opts is assumed to be alphabetically sorted.
>> virtual void printOptions(StrOptionPairVector &Opts, size_t
>> MaxArgLen) {
>> for (size_t i = 0, e = Opts.size(); i != e; ++i)
>> Opts[i].second->printOptionInfo(MaxArgLen);
>> }
>>
>> + void printSubCommands(StrSubCommandPairVector &Subs, size_t
>> MaxSubLen) {
>> + for (const auto &S : Subs) {
>> + outs() << " " << S.first;
>> + if (S.second->getDescription()) {
>> + outs().indent(MaxSubLen - strlen(S.first));
>> + outs() << " - " << S.second->getDescription();
>> + }
>> + outs() << "\n";
>> + }
>> + }
>> +
>> public:
>> explicit HelpPrinter(bool showHidden) : ShowHidden(showHidden) {}
>> virtual ~HelpPrinter() {}
>> @@ -1510,23 +1751,56 @@ public:
>> if (!Value)
>> return;
>>
>> + SubCommand *Sub = GlobalParser->getActiveSubCommand();
>> + auto &OptionsMap = Sub->OptionsMap;
>> + auto &PositionalOpts = Sub->PositionalOpts;
>> + auto &ConsumeAfterOpt = Sub->ConsumeAfterOpt;
>> +
>> StrOptionPairVector Opts;
>> - sortOpts(GlobalParser->OptionsMap, Opts, ShowHidden);
>> + sortOpts(OptionsMap, Opts, ShowHidden);
>> +
>> + StrSubCommandPairVector Subs;
>> + sortSubCommands(GlobalParser->RegisteredSubCommands, Subs);
>>
>>
We are checking `Subs.size() > 2` below, but `sortSubCommands` filters out
the subcommands without a name, i.e.
the two builtin special subcommand instances. Thus the logic in the
`Subs.size() > 2` check is invalid.
I'll try working on a patch to fix this,
Alex
>
>> if (GlobalParser->ProgramOverview)
>> outs() << "OVERVIEW: " << GlobalParser->ProgramOverview << "\n";
>>
>> - outs() << "USAGE: " << GlobalParser->ProgramName << " [options]";
>> + if (Sub == &*TopLevelSubCommand)
>> + outs() << "USAGE: " << GlobalParser->ProgramName
>> + << " [subcommand] [options]";
>> + else {
>> + if (Sub->getDescription() != nullptr) {
>> + outs() << "SUBCOMMAND '" << Sub->getName()
>> + << "': " << Sub->getDescription() << "\n\n";
>> + }
>> + outs() << "USAGE: " << GlobalParser->ProgramName << " " <<
>> Sub->getName()
>> + << " [options]";
>> + }
>>
>> - for (auto Opt : GlobalParser->PositionalOpts) {
>> + for (auto Opt : PositionalOpts) {
>> if (Opt->hasArgStr())
>> outs() << " --" << Opt->ArgStr;
>> outs() << " " << Opt->HelpStr;
>> }
>>
>> // Print the consume after option info if it exists...
>> - if (GlobalParser->ConsumeAfterOpt)
>> - outs() << " " << GlobalParser->ConsumeAfterOpt->HelpStr;
>> + if (ConsumeAfterOpt)
>> + outs() << " " << ConsumeAfterOpt->HelpStr;
>> +
>> + if (Sub == &*TopLevelSubCommand && Subs.size() > 2) {
>>
>>
>> I'm sorry if I missed something, but I looked at the review for this
>> commit, and couldn't find an answer to one question that I have, so I hope
>> that you can help me:
>>
>> Is the > 2 intentional here? I think because of this check subcommands
>> don't show up in help unless there are 3 of them. It seems like a bug to
>> me, since it's valid to have a tool with just 2 subcommands.
>>
>> Thanks,
>> Alex
>>
>> + // Compute the maximum subcommand length...
>> + size_t MaxSubLen = 0;
>> + for (size_t i = 0, e = Subs.size(); i != e; ++i)
>> + MaxSubLen = std::max(MaxSubLen, strlen(Subs[i].first));
>> +
>> + outs() << "\n\n";
>> + outs() << "SUBCOMMANDS:\n\n";
>> + printSubCommands(Subs, MaxSubLen);
>> + outs() << "\n";
>> + outs() << " Type \"" << GlobalParser->ProgramName
>> + << " <subcommand> -help\" to get more help on a specific "
>> + "subcommand";
>> + }
>>
>> outs() << "\n\n";
>>
>> @@ -1675,12 +1949,13 @@ static cl::opt<HelpPrinter, true, parser
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161010/ff34c9a9/attachment.html>
More information about the llvm-commits
mailing list