[llvm] r274171 - Resubmit "Update llvm command line parser to support subcommands."
Zachary Turner via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 10 11:15:04 PDT 2016
Ahh, nice catch. Thanks for digging in.
On Mon, Oct 10, 2016 at 10:21 AM Alex L <arphaman at gmail.com> wrote:
> 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/66b4a76f/attachment.html>
More information about the llvm-commits
mailing list