[llvm] r353048 - [CommandLine] Don't print empty sentinel values from EnumValN lists in help text
James Henderson via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 4 06:48:33 PST 2019
Author: jhenderson
Date: Mon Feb 4 06:48:33 2019
New Revision: 353048
URL: http://llvm.org/viewvc/llvm-project?rev=353048&view=rev
Log:
[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 '=<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.
-some-option - some help text
-some-option=<value> - some help text
=v1 - description 1
=v2 - description 2
3) Otherwise empty-named options are printed as =<empty> rather than
simply '='.
4) Option values without help text do not have the '-' separator
printed.
-some-option=<value> - some help text
=v1 - description 1
=v2
=<empty> - 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 r352750.
Reviewed by: ruiu, thopre, mstorsjo
Differential Revision: https://reviews.llvm.org/D57030
Modified:
llvm/trunk/lib/Support/CommandLine.cpp
llvm/trunk/tools/llvm-symbolizer/llvm-symbolizer.cpp
llvm/trunk/unittests/Support/CommandLineTest.cpp
Modified: llvm/trunk/lib/Support/CommandLine.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=353048&r1=353047&r2=353048&view=diff
==============================================================================
--- llvm/trunk/lib/Support/CommandLine.cpp (original)
+++ llvm/trunk/lib/Support/CommandLine.cpp Mon Feb 4 06:48:33 2019
@@ -1469,17 +1469,23 @@ static StringRef getValueStr(const Optio
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<StringRef, StringRef> 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 Help
// 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
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
//
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(
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
return e;
}
+static StringRef EqValue = "=<value>";
+static StringRef EmptyOption = "<empty>";
+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::getOptionWid
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())
Modified: llvm/trunk/tools/llvm-symbolizer/llvm-symbolizer.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-symbolizer/llvm-symbolizer.cpp?rev=353048&r1=353047&r2=353048&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-symbolizer/llvm-symbolizer.cpp (original)
+++ llvm/trunk/tools/llvm-symbolizer/llvm-symbolizer.cpp Mon Feb 4 06:48:33 2019
@@ -38,7 +38,7 @@ ClUseSymbolTable("use-symbol-table", cl:
static cl::opt<FunctionNameKind> 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"),
Modified: llvm/trunk/unittests/Support/CommandLineTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CommandLineTest.cpp?rev=353048&r1=353047&r2=353048&view=diff
==============================================================================
--- llvm/trunk/unittests/Support/CommandLineTest.cpp (original)
+++ llvm/trunk/unittests/Support/CommandLineTest.cpp Mon Feb 4 06:48:33 2019
@@ -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, GetCommandLineArgu
}
#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
+}
+
+class GetOptionWidthTest : public ::testing::Test {
+public:
+ enum class OptionValue { Val };
+
+ template <typename... Ts>
+ size_t runTest(StringRef ArgName, Ts... OptionAttributes) {
+ StackOption<OptionValue> 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");
+ Twine ExpectedStr = " -" + ArgName + "=<value> - ";
+ EXPECT_EQ(
+ runTest(ArgName, cl::values(clEnumValN(OptionValue::Val, "v", "help"))),
+ ExpectedStr.str().size());
+}
+
+TEST_F(GetOptionWidthTest, GetOptionWidthFirstOptionNameLonger) {
+ StringRef OptName("a-long-option-name");
+ Twine ExpectedStr = " =" + OptName + " - ";
+ EXPECT_EQ(
+ runTest("a", cl::values(clEnumValN(OptionValue::Val, OptName, "help"),
+ clEnumValN(OptionValue::Val, "b", "help"))),
+ ExpectedStr.str().size());
+}
+
+TEST_F(GetOptionWidthTest, GetOptionWidthSecondOptionNameLonger) {
+ StringRef OptName("a-long-option-name");
+ Twine ExpectedStr = " =" + OptName + " - ";
+ EXPECT_EQ(
+ runTest("a", cl::values(clEnumValN(OptionValue::Val, "b", "help"),
+ clEnumValN(OptionValue::Val, OptName, "help"))),
+ ExpectedStr.str().size());
+}
+
+TEST_F(GetOptionWidthTest, GetOptionWidthEmptyOptionNameLonger) {
+ Twine ExpectedStr = " =<empty> - ";
+ // The length of a=<value> (including indentation) is actually the same as the
+ // =<empty> 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"))),
+ ExpectedStr.str().size());
+}
+
+TEST_F(GetOptionWidthTest,
+ GetOptionWidthValueOptionalEmptyOptionWithNoDescription) {
+ StringRef ArgName("a");
+ // The length of a=<value> (including indentation) is actually the same as the
+ // =<empty> 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.
+ Twine ExpectedStr = " -" + ArgName + "=<value> - ";
+ EXPECT_EQ(runTest(ArgName, cl::ValueOptional,
+ cl::values(clEnumValN(OptionValue::Val, "value", "help"),
+ clEnumValN(OptionValue::Val, "", ""))),
+ ExpectedStr.str().size());
+}
+
+TEST_F(GetOptionWidthTest,
+ GetOptionWidthValueRequiredEmptyOptionWithNoDescription) {
+ // The length of a=<value> (including indentation) is actually the same as the
+ // =<empty> string, so it is impossible to distinguish via testing the case
+ // where the empty string is picked from where the option name is picked
+ Twine ExpectedStr = " =<empty> - ";
+ EXPECT_EQ(runTest("a", cl::ValueRequired,
+ cl::values(clEnumValN(OptionValue::Val, "value", "help"),
+ clEnumValN(OptionValue::Val, "", ""))),
+ ExpectedStr.str().size());
+}
+
} // anonymous namespace
More information about the llvm-commits
mailing list