From 6277ca3b6d99dd917876a839ce5a5039d5f253cf Mon Sep 17 00:00:00 2001 From: James Henderson Date: Mon, 4 Feb 2019 16:17:57 +0000 Subject: [PATCH] [CommandLine] Don't print empty sentinel values from EnumValN lists in help text In order to make an option value truly optional, both the ValueOptional attribute and an empty-named value are required. Prior to this change, this empty-named value appears in the command-line help text: -some-option - some help text =v1 - description 1 =v2 - description 2 = - 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. -some-option - some help text -some-option= - some help text =v1 - description 1 =v2 - description 2 3) Otherwise empty-named options are printed as = rather than simply '='. 4) Option values without help text do not have the '-' separator printed. -some-option= - some help text =v1 - description 1 =v2 = - description It also tweaks the llvm-symbolizer -functions help text to not print a trailing ':' as that looks bad combined with 1) above. This is mostly a reland of r353048 which in turn was a reland of r352750. Reviewed by: ruiu, thopre, mstorsjo Differential Revision: https://reviews.llvm.org/D57030 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@353053 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Support/CommandLine.cpp | 79 ++++++-- tools/llvm-symbolizer/llvm-symbolizer.cpp | 2 +- unittests/Support/CommandLineTest.cpp | 215 ++++++++++++++++++++++ 3 files changed, 279 insertions(+), 17 deletions(-) diff --git a/lib/Support/CommandLine.cpp b/lib/Support/CommandLine.cpp index f8bc6a8f615..52f1c0e007b 100644 --- a/lib/Support/CommandLine.cpp +++ b/lib/Support/CommandLine.cpp @@ -1469,17 +1469,23 @@ static StringRef getValueStr(const Option &O, StringRef DefaultMsg) { return O.ValueStr; } +static StringRef ArgPrefix = " -"; +static StringRef ArgHelpPrefix = " - "; +static size_t ArgPrefixesSize = ArgPrefix.size() + ArgHelpPrefix.size(); + //===----------------------------------------------------------------------===// // cl::alias class implementation // // Return the width of the option tag for printing... -size_t alias::getOptionWidth() const { return ArgStr.size() + 6; } +size_t alias::getOptionWidth() const { return ArgStr.size() + ArgPrefixesSize; } void Option::printHelpStr(StringRef HelpStr, size_t Indent, - size_t FirstLineIndentedBy) { + size_t FirstLineIndentedBy) { + assert(Indent >= FirstLineIndentedBy); std::pair Split = HelpStr.split('\n'); - outs().indent(Indent - FirstLineIndentedBy) << " - " << Split.first << "\n"; + outs().indent(Indent - FirstLineIndentedBy) + << ArgHelpPrefix << Split.first << "\n"; while (!Split.second.empty()) { Split = Split.second.split('\n'); outs().indent(Indent) << Split.first << "\n"; @@ -1488,8 +1494,8 @@ void Option::printHelpStr(StringRef HelpStr, size_t Indent, // Print out the option for the alias. void alias::printOptionInfo(size_t GlobalWidth) const { - outs() << " -" << ArgStr; - printHelpStr(HelpStr, GlobalWidth, ArgStr.size() + 6); + outs() << ArgPrefix << ArgStr; + printHelpStr(HelpStr, GlobalWidth, ArgStr.size() + ArgPrefixesSize); } //===----------------------------------------------------------------------===// @@ -1510,7 +1516,7 @@ size_t basic_parser_impl::getOptionWidth(const Option &O) const { Len += getValueStr(O, ValName).size() + FormattingLen; } - return Len + 6; + return Len + ArgPrefixesSize; } // printOptionInfo - Print out information about this option. The @@ -1518,7 +1524,7 @@ size_t basic_parser_impl::getOptionWidth(const Option &O) const { // void basic_parser_impl::printOptionInfo(const Option &O, size_t GlobalWidth) const { - outs() << " -" << O.ArgStr; + outs() << ArgPrefix << O.ArgStr; auto ValName = getValueName(); if (!ValName.empty()) { @@ -1534,7 +1540,7 @@ void basic_parser_impl::printOptionInfo(const Option &O, void basic_parser_impl::printOptionName(const Option &O, size_t GlobalWidth) const { - outs() << " -" << O.ArgStr; + outs() << ArgPrefix << O.ArgStr; outs().indent(GlobalWidth - O.ArgStr.size()); } @@ -1642,12 +1648,28 @@ unsigned generic_parser_base::findOption(StringRef Name) { return e; } +static StringRef EqValue = "="; +static StringRef EmptyOption = ""; +static StringRef OptionPrefix = " ="; +static size_t OptionPrefixesSize = OptionPrefix.size() + ArgHelpPrefix.size(); + +static bool shouldPrintOption(StringRef Name, StringRef Description, + const Option &O) { + return O.getValueExpectedFlag() != ValueOptional || !Name.empty() || + !Description.empty(); +} + // Return the width of the option tag for printing... size_t generic_parser_base::getOptionWidth(const Option &O) const { if (O.hasArgStr()) { - size_t Size = O.ArgStr.size() + 6; - for (unsigned i = 0, e = getNumOptions(); i != e; ++i) - Size = std::max(Size, getOption(i).size() + 8); + size_t Size = O.ArgStr.size() + ArgPrefixesSize + EqValue.size(); + for (unsigned i = 0, e = getNumOptions(); i != e; ++i) { + StringRef Name = getOption(i); + if (!shouldPrintOption(Name, getDescription(i), O)) + continue; + size_t NameSize = Name.empty() ? EmptyOption.size() : Name.size(); + Size = std::max(Size, NameSize + OptionPrefixesSize); + } return Size; } else { size_t BaseSize = 0; @@ -1663,13 +1685,38 @@ 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() << ArgPrefix << O.ArgStr; + Option::printHelpStr(O.HelpStr, GlobalWidth, + O.ArgStr.size() + ArgPrefixesSize); + break; + } + } + } + outs() << ArgPrefix << O.ArgStr << EqValue; + Option::printHelpStr(O.HelpStr, GlobalWidth, + O.ArgStr.size() + EqValue.size() + ArgPrefixesSize); 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 OptionName = getOption(i); + StringRef Description = getDescription(i); + if (!shouldPrintOption(OptionName, Description, O)) + continue; + assert(GlobalWidth >= OptionName.size() + OptionPrefixesSize); + size_t NumSpaces = GlobalWidth - OptionName.size() - OptionPrefixesSize; + outs() << OptionPrefix << OptionName; + if (OptionName.empty()) { + outs() << EmptyOption; + assert(NumSpaces >= EmptyOption.size()); + NumSpaces -= EmptyOption.size(); + } + if (!Description.empty()) + outs().indent(NumSpaces) << ArgHelpPrefix << " " << Description; + outs() << '\n'; } } else { if (!O.HelpStr.empty()) diff --git a/tools/llvm-symbolizer/llvm-symbolizer.cpp b/tools/llvm-symbolizer/llvm-symbolizer.cpp index 15b2b9dddd4..0b0b8a68a4a 100644 --- a/tools/llvm-symbolizer/llvm-symbolizer.cpp +++ b/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/unittests/Support/CommandLineTest.cpp b/unittests/Support/CommandLineTest.cpp index 416e0eef5bf..150563b5b0b 100644 --- a/unittests/Support/CommandLineTest.cpp +++ b/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,218 @@ 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 +} + +class GetOptionWidthTest : public ::testing::Test { +public: + enum class OptionValue { Val }; + + template + size_t runTest(StringRef ArgName, Ts... OptionAttributes) { + StackOption TestOption(ArgName, cl::desc("some help"), + OptionAttributes...); + return getOptionWidth(TestOption); + } + +private: + // This is a workaround for cl::Option sub-classes having their + // printOptionInfo + // functions private. + size_t getOptionWidth(const cl::Option &O) { return O.getOptionWidth(); } +}; + +TEST_F(GetOptionWidthTest, GetOptionWidthArgNameLonger) { + StringRef ArgName("a-long-argument-name"); + size_t ExpectedStrSize = (" -" + ArgName + "= - ").str().size(); + EXPECT_EQ( + runTest(ArgName, cl::values(clEnumValN(OptionValue::Val, "v", "help"))), + ExpectedStrSize); +} + +TEST_F(GetOptionWidthTest, GetOptionWidthFirstOptionNameLonger) { + StringRef OptName("a-long-option-name"); + size_t ExpectedStrSize = (" =" + OptName + " - ").str().size(); + EXPECT_EQ( + runTest("a", cl::values(clEnumValN(OptionValue::Val, OptName, "help"), + clEnumValN(OptionValue::Val, "b", "help"))), + ExpectedStrSize); +} + +TEST_F(GetOptionWidthTest, GetOptionWidthSecondOptionNameLonger) { + StringRef OptName("a-long-option-name"); + size_t ExpectedStrSize = (" =" + OptName + " - ").str().size(); + EXPECT_EQ( + runTest("a", cl::values(clEnumValN(OptionValue::Val, "b", "help"), + clEnumValN(OptionValue::Val, OptName, "help"))), + ExpectedStrSize); +} + +TEST_F(GetOptionWidthTest, GetOptionWidthEmptyOptionNameLonger) { + size_t ExpectedStrSize = StringRef(" = - ").size(); + // The length of a= (including indentation) is actually the same as the + // = string, so it is impossible to distinguish via testing the case + // where the empty string is picked from where the option name is picked. + EXPECT_EQ(runTest("a", cl::values(clEnumValN(OptionValue::Val, "b", "help"), + clEnumValN(OptionValue::Val, "", "help"))), + ExpectedStrSize); +} + +TEST_F(GetOptionWidthTest, + GetOptionWidthValueOptionalEmptyOptionWithNoDescription) { + StringRef ArgName("a"); + // The length of a= (including indentation) is actually the same as the + // = string, so it is impossible to distinguish via testing the case + // where the empty string is ignored from where it is not ignored. + // The dash will not actually be printed, but the space it would take up is + // included to ensure a consistent column width. + size_t ExpectedStrSize = (" -" + ArgName + "= - ").str().size(); + EXPECT_EQ(runTest(ArgName, cl::ValueOptional, + cl::values(clEnumValN(OptionValue::Val, "value", "help"), + clEnumValN(OptionValue::Val, "", ""))), + ExpectedStrSize); +} + +TEST_F(GetOptionWidthTest, + GetOptionWidthValueRequiredEmptyOptionWithNoDescription) { + // The length of a= (including indentation) is actually the same as the + // = string, so it is impossible to distinguish via testing the case + // where the empty string is picked from where the option name is picked + size_t ExpectedStrSize = StringRef(" = - ").size(); + EXPECT_EQ(runTest("a", cl::ValueRequired, + cl::values(clEnumValN(OptionValue::Val, "value", "help"), + clEnumValN(OptionValue::Val, "", ""))), + ExpectedStrSize); +} + } // anonymous namespace