From 140f75f625bc815c5c7c73a6ba765d49998f95c4 Mon Sep 17 00:00:00 2001 From: James Henderson Date: Thu, 31 Jan 2019 13:58:48 +0000 Subject: [PATCH] [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 '=' 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 = 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 --- llvm/lib/Support/CommandLine.cpp | 32 ++++- .../tools/llvm-symbolizer/llvm-symbolizer.cpp | 2 +- llvm/unittests/Support/CommandLineTest.cpp | 134 ++++++++++++++++++ 3 files changed, 162 insertions(+), 6 deletions(-) diff --git a/llvm/lib/Support/CommandLine.cpp b/llvm/lib/Support/CommandLine.cpp index f8bc6a8f615c3..886595e30a50a 100644 --- a/llvm/lib/Support/CommandLine.cpp +++ b/llvm/lib/Support/CommandLine.cpp @@ -1663,13 +1663,35 @@ size_t generic_parser_base::getOptionWidth(const Option &O) const { void generic_parser_base::printOptionInfo(const Option &O, size_t GlobalWidth) const { if (O.hasArgStr()) { - outs() << " -" << O.ArgStr; - Option::printHelpStr(O.HelpStr, GlobalWidth, O.ArgStr.size() + 6); + // When the value is optional, first print a line just describing the option + // without values. + if (O.getValueExpectedFlag() == ValueOptional) { + for (unsigned i = 0, e = getNumOptions(); i != e; ++i) { + if (getOption(i).empty()) { + outs() << " -" << O.ArgStr; + Option::printHelpStr(O.HelpStr, GlobalWidth, O.ArgStr.size() + 6); + break; + } + } + } + outs() << " -" << O.ArgStr << "="; + Option::printHelpStr(O.HelpStr, GlobalWidth, O.ArgStr.size() + 14); for (unsigned i = 0, e = getNumOptions(); i != e; ++i) { - size_t NumSpaces = GlobalWidth - getOption(i).size() - 8; - outs() << " =" << getOption(i); - outs().indent(NumSpaces) << " - " << getDescription(i) << '\n'; + StringRef ValueName = getOption(i); + StringRef Description = getDescription(i); + if (O.getValueExpectedFlag() == ValueOptional && ValueName.empty() && + Description.empty()) + continue; + size_t NumSpaces = GlobalWidth - ValueName.size() - 8; + outs() << " =" << ValueName; + if (ValueName.empty()) { + outs() << ""; + NumSpaces -= 7; + } + if (!Description.empty()) + outs().indent(NumSpaces) << " - " << Description; + outs() << '\n'; } } else { if (!O.HelpStr.empty()) diff --git a/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp b/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp index 15b2b9dddd430..0b0b8a68a4ae7 100644 --- a/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp +++ b/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp @@ -38,7 +38,7 @@ ClUseSymbolTable("use-symbol-table", cl::init(true), static cl::opt ClPrintFunctions( "functions", cl::init(FunctionNameKind::LinkageName), - cl::desc("Print function name for a given address:"), cl::ValueOptional, + cl::desc("Print function name for a given address"), cl::ValueOptional, cl::values(clEnumValN(FunctionNameKind::None, "none", "omit function name"), clEnumValN(FunctionNameKind::ShortName, "short", "print short function name"), diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp index 416e0eef5bf78..8f8e1bad7b66a 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 FilePath; + bool Valid = true; + +private: + int RedirectFD; + int OldFD; + int NewFD; +}; + +struct AutoDeleteFile { + SmallVector 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 std::string runTest(Ts... OptionAttributes) { + AutoDeleteFile File; + { + OutputRedirector Stdout(fileno(stdout)); + if (!Stdout.Valid) + return ""; + File.FilePath = Stdout.FilePath; + + StackOption 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 + "= - " + 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 + "= - " + 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 + "= - " + HelpText + "\n" + " =v1 - desc1\n" + " = - 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 + "= - " + HelpText + "\n" + " =v1 - desc1\n" + " =\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 + "= - " + HelpText + "\n" + " =v1\n").str()); + // clang-format on +} + } // anonymous namespace