Skip to content

Commit

Permalink
[CommandLine] Don't print empty sentinel values from EnumValN lists i…
Browse files Browse the repository at this point in the history 
…n 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

llvm-svn: 353048
  • Loading branch information
@jh7370
jh7370 committed Feb 4, 2019
1 parent eef758e commit d90b5a2
Show file tree
Hide file tree
Showing 3 changed files with 279 additions and 17 deletions.
79 changes: 63 additions & 16 deletions llvm/lib/Support/CommandLine.cpp
View file
Expand Up @@ -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<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";
Expand All @@ -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);
}

//===----------------------------------------------------------------------===//
Expand All @@ -1510,15 +1516,15 @@ 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
// to-be-maintained width is specified.
//
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()) {
Expand All @@ -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());
}

Expand Down Expand Up @@ -1642,12 +1648,28 @@ unsigned generic_parser_base::findOption(StringRef Name) {
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;
Expand All @@ -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())
Expand Down
2 changes: 1 addition & 1 deletion llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
View file
Expand Up @@ -38,7 +38,7 @@ ClUseSymbolTable("use-symbol-table", cl::init(true),

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"),
Expand Down
215 changes: 215 additions & 0 deletions llvm/unittests/Support/CommandLineTest.cpp
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -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<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

0 comments on commit d90b5a2

Please sign in to comment.