[CommandLine] Show '[subcommand]' in the help for less than 3 subcommands

[igorkudrin]

When a tool defines only one or two subcommands, the [subcommand] part is not displayed in the USAGE help line. Note that a similar issue with printing the list of the subcommands has been fixed in https://reviews.llvm.org/D25463.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Igor Kudrin (igorkudrin)

Changes

When a tool defines only one or two subcommands, the [subcommand] part is not displayed in the USAGE help line. Note that a similar issue with printing the list of the subcommands has been fixed in https://reviews.llvm.org/D25463.

---

Full diff: https://github.com/llvm/llvm-project/pull/74557.diff

1 Files Affected:

• (modified) llvm/lib/Support/CommandLine.cpp (+1-1)

diff --git a/llvm/lib/Support/CommandLine.cpp b/llvm/lib/Support/CommandLine.cpp
index a7e0cae8b855d..31f79972125da 100644
--- a/llvm/lib/Support/CommandLine.cpp
+++ b/llvm/lib/Support/CommandLine.cpp
@@ -2372,7 +2372,7 @@ class HelpPrinter {
 
     if (Sub == &SubCommand::getTopLevel()) {
       outs() << "USAGE: " << GlobalParser->ProgramName;
-      if (Subs.size() > 2)
+      if (!Subs.empty())
         outs() << " [subcommand]";
       outs() << " [options]";
     } else {

[igorkudrin]

I can't find an easy way to test this change. cl::PrintHelpMessage() uses llvm::outs() unconditionally; changing it to take an output stream as an argument would cause a cascade of changes in all the methods it calls directly and indirectly, including, for example, Option::printOptionInfo() with its overrides, and many others. The patch doesn't seem worth such a massive change.

PS The buildbot failures do not seem relevant, as the patch could not cause crashes.

[dwblaikie]

I can't find an easy way to test this change. cl::PrintHelpMessage() uses llvm::outs() unconditionally; changing it to take an output stream as an argument would cause a cascade of changes in all the methods it calls directly and indirectly, including, for example, Option::printOptionInfo() with its overrides, and many others. The patch doesn't seem worth such a massive change.

I guess more unit-testability would be nice to have sooner or later as this library gets wider adoption. Whether or not you want to sign up for that now is a more open question, probably.

How were similar changes tested? Perhaps tested with a concrete usage like clang, etc? Maybe one of these users has a case that would demonstrate the change/fix?

[igorkudrin]

Hmm, I was under the impression that gtest runs unit tests in parallel, so it is not possible to intercept stdout. But in fact, gtest is single-threaded, and moreover, there are other tests in the test suite that already intercept the stream. So I added tests for the change to the patch.

[jh7370]

LGTM. Strange that the original patch (79f3333) used > 2 for "at least 1". I wonder if some refactoring later caused the issue?

[igorkudrin]

LGTM. Strange that the original patch (79f3333) used > 2 for "at least 1". I wonder if some refactoring later caused the issue?

Thanks. I guess that it was made to match the condition (Sub == &*TopLevelSubCommand && Subs.size() > 2) a few lines later. And (just guessing again) the original idea about Subs.size() > 2 might have been based on the assumption that two special subcommands, TopLevel and All, would also be added to the list. And all this proves once again the importance of testing.