[CommandLine] Improve help text for cl::values style options

In order to make an option value truly optional, both the ValueOptional
and an empty-named value are required. This empty-named value appears in
the command-line help text, which is not ideal.

This change improves the help text for these sort of options in a number
of ways:
1) ValueOptional options with an empty-named value now print their help
text twice: both without and then with '=<value>' after the name. The
latter version then lists the allowed values after it.
2) Empty-named values with no help text in ValueOptional options are not
listed in the permitted values.
3) Otherwise empty-named options are printed as =<empty> rather than
simply '='.
4) Option values without help text do not have the '-' separator
printed.

It also tweaks the llvm-symbolizer -functions help text to not print a
trailing ':' as that looks bad combined with 1) above.

Reviewed by: thopre, ruiu

Differential Revision: https://reviews.llvm.org/D57030

llvm-svn: 352750

1 files changed, 134 insertions, 0 deletions

diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp
index 416e0ee..8f8e1ba 100644
--- a/llvm/unittests/Support/CommandLineTest.cpp
+++ b/llvm/unittests/Support/CommandLineTest.cpp
@@ -13,6 +13,7 @@
 #include "llvm/Config/config.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/StringSaver.h"
@@ -839,4 +840,137 @@ TEST(CommandLineTest, GetCommandLineArguments) {
 }
 #endif
 
+class OutputRedirector {
+public:
+  OutputRedirector(int RedirectFD)
+      : RedirectFD(RedirectFD), OldFD(dup(RedirectFD)) {
+    if (OldFD == -1 ||
+        sys::fs::createTemporaryFile("unittest-redirect", "", NewFD,
+                                     FilePath) ||
+        dup2(NewFD, RedirectFD) == -1)
+      Valid = false;
+  }
+
+  ~OutputRedirector() {
+    dup2(OldFD, RedirectFD);
+    close(OldFD);
+    close(NewFD);
+  }
+
+  SmallVector<char, 128> FilePath;
+  bool Valid = true;
+
+private:
+  int RedirectFD;
+  int OldFD;
+  int NewFD;
+};
+
+struct AutoDeleteFile {
+  SmallVector<char, 128> FilePath;
+  ~AutoDeleteFile() {
+    if (!FilePath.empty())
+      sys::fs::remove(std::string(FilePath.data(), FilePath.size()));
+  }
+};
+
+class PrintOptionInfoTest : public ::testing::Test {
+public:
+  // Return std::string because the output of a failing EXPECT check is
+  // unreadable for StringRef. It also avoids any lifetime issues.
+  template <typename... Ts> std::string runTest(Ts... OptionAttributes) {
+    AutoDeleteFile File;
+    {
+      OutputRedirector Stdout(fileno(stdout));
+      if (!Stdout.Valid)
+        return "";
+      File.FilePath = Stdout.FilePath;
+
+      StackOption<OptionValue> TestOption(Opt, cl::desc(HelpText),
+                                          OptionAttributes...);
+      printOptionInfo(TestOption, 25);
+      outs().flush();
+    }
+    auto Buffer = MemoryBuffer::getFile(File.FilePath);
+    if (!Buffer)
+      return "";
+    return Buffer->get()->getBuffer().str();
+  }
+
+  enum class OptionValue { Val };
+  const StringRef Opt = "some-option";
+  const StringRef HelpText = "some help";
+
+private:
+  // This is a workaround for cl::Option sub-classes having their
+  // printOptionInfo functions private.
+  void printOptionInfo(const cl::Option &O, size_t Width) {
+    O.printOptionInfo(Width);
+  }
+};
+
+TEST_F(PrintOptionInfoTest, PrintOptionInfoValueOptionalWithoutSentinel) {
+  std::string Output =
+      runTest(cl::ValueOptional,
+              cl::values(clEnumValN(OptionValue::Val, "v1", "desc1")));
+
+  // clang-format off
+  EXPECT_EQ(Output, ("  -" + Opt + "=<value> - " + HelpText + "\n"
+                     "    =v1                -   desc1\n")
+                        .str());
+  // clang-format on
+}
+
+TEST_F(PrintOptionInfoTest, PrintOptionInfoValueOptionalWithSentinel) {
+  std::string Output = runTest(
+      cl::ValueOptional, cl::values(clEnumValN(OptionValue::Val, "v1", "desc1"),
+                                    clEnumValN(OptionValue::Val, "", "")));
+
+  // clang-format off
+  EXPECT_EQ(Output,
+            ("  -" + Opt + "         - " + HelpText + "\n"
+             "  -" + Opt + "=<value> - " + HelpText + "\n"
+             "    =v1                -   desc1\n")
+                .str());
+  // clang-format on
+}
+
+TEST_F(PrintOptionInfoTest, PrintOptionInfoValueOptionalWithSentinelWithHelp) {
+  std::string Output = runTest(
+      cl::ValueOptional, cl::values(clEnumValN(OptionValue::Val, "v1", "desc1"),
+                                    clEnumValN(OptionValue::Val, "", "desc2")));
+
+  // clang-format off
+  EXPECT_EQ(Output, ("  -" + Opt + "         - " + HelpText + "\n"
+                     "  -" + Opt + "=<value> - " + HelpText + "\n"
+                     "    =v1                -   desc1\n"
+                     "    =<empty>           -   desc2\n")
+                        .str());
+  // clang-format on
+}
+
+TEST_F(PrintOptionInfoTest, PrintOptionInfoValueRequiredWithEmptyValueName) {
+  std::string Output = runTest(
+      cl::ValueRequired, cl::values(clEnumValN(OptionValue::Val, "v1", "desc1"),
+                                    clEnumValN(OptionValue::Val, "", "")));
+
+  // clang-format off
+  EXPECT_EQ(Output, ("  -" + Opt + "=<value> - " + HelpText + "\n"
+                     "    =v1                -   desc1\n"
+                     "    =<empty>\n")
+                        .str());
+  // clang-format on
+}
+
+TEST_F(PrintOptionInfoTest, PrintOptionInfoEmptyValueDescription) {
+  std::string Output = runTest(
+      cl::ValueRequired, cl::values(clEnumValN(OptionValue::Val, "v1", "")));
+
+  // clang-format off
+  EXPECT_EQ(Output,
+            ("  -" + Opt + "=<value> - " + HelpText + "\n"
+             "    =v1\n").str());
+  // clang-format on
+}
+
 }  // anonymous namespace