Skip to content

Fix logic to detect cl::option equality. #65754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 10, 2023
Merged

Fix logic to detect cl::option equality. #65754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions llvm/include/llvm/Support/CommandLine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,7 @@ struct OptionValueBase : public GenericOptionValue {
// Some options may take their value from a different data type.
template <class DT> void setValue(const DT & /*V*/) {}

// Returns whether this instance matches the argument.
bool compare(const DataType & /*V*/) const { return false; }

bool compare(const GenericOptionValue & /*V*/) const override {
Expand Down Expand Up @@ -587,7 +588,8 @@ template <class DataType> class OptionValueCopy : public GenericOptionValue {
Value = V;
}

bool compare(const DataType &V) const { return Valid && (Value != V); }
// Returns whether this instance matches V.
bool compare(const DataType &V) const { return Valid && (Value == V); }

bool compare(const GenericOptionValue &V) const override {
const OptionValueCopy<DataType> &VC =
Expand Down Expand Up @@ -1442,7 +1444,7 @@ class opt
}

void printOptionValue(size_t GlobalWidth, bool Force) const override {
if (Force || this->getDefault().compare(this->getValue())) {
if (Force || !this->getDefault().compare(this->getValue())) {
cl::printOptionDiff<ParserClass>(*this, Parser, this->getValue(),
this->getDefault(), GlobalWidth);
}
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Support/CommandLine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2181,15 +2181,15 @@ void generic_parser_base::printGenericOptionDiff(

unsigned NumOpts = getNumOptions();
for (unsigned i = 0; i != NumOpts; ++i) {
if (Value.compare(getOptionValue(i)))
if (!Value.compare(getOptionValue(i)))
continue;

outs() << "= " << getOption(i);
size_t L = getOption(i).size();
size_t NumSpaces = MaxOptWidth > L ? MaxOptWidth - L : 0;
outs().indent(NumSpaces) << " (default: ";
for (unsigned j = 0; j != NumOpts; ++j) {
if (Default.compare(getOptionValue(j)))
if (!Default.compare(getOptionValue(j)))
continue;
outs() << getOption(j);
break;
Expand Down
49 changes: 40 additions & 9 deletions llvm/unittests/Support/CommandLineTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1294,7 +1294,8 @@ struct AutoDeleteFile {
}
};

class PrintOptionInfoTest : public ::testing::Test {
template <void (*Func)(const cl::Option &)>
class PrintOptionTestBase : 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.
Expand All @@ -1309,7 +1310,7 @@ class PrintOptionInfoTest : public ::testing::Test {

StackOption<OptionValue> TestOption(Opt, cl::desc(HelpText),
OptionAttributes...);
printOptionInfo(TestOption, 26);
Func(TestOption);
outs().flush();
}
auto Buffer = MemoryBuffer::getFile(File.FilePath);
Expand All @@ -1321,14 +1322,15 @@ class PrintOptionInfoTest : public ::testing::Test {
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);
}
};
void printOptionInfo(const cl::Option &O) {
O.printOptionInfo(/*GlobalWidth=*/26);
}

using PrintOptionInfoTest = PrintOptionTestBase<printOptionInfo>;

TEST_F(PrintOptionInfoTest, PrintOptionInfoValueOptionalWithoutSentinel) {
std::string Output =
Expand Down Expand Up @@ -1402,7 +1404,7 @@ TEST_F(PrintOptionInfoTest, PrintOptionInfoMultilineValueDescription) {
"which has a really long description\n"
"thus it is multi-line."),
clEnumValN(OptionValue::Val, "",
"This is an unnamed enum value option\n"
"This is an unnamed enum value\n"
"Should be indented as well")));

// clang-format off
Expand All @@ -1411,11 +1413,40 @@ TEST_F(PrintOptionInfoTest, PrintOptionInfoMultilineValueDescription) {
" =v1 - This is the first enum value\n"
" which has a really long description\n"
" thus it is multi-line.\n"
" =<empty> - This is an unnamed enum value option\n"
" =<empty> - This is an unnamed enum value\n"
" Should be indented as well\n").str());
// clang-format on
}

void printOptionValue(const cl::Option &O) {
O.printOptionValue(/*GlobalWidth=*/12, /*Force=*/true);
}

using PrintOptionValueTest = PrintOptionTestBase<printOptionValue>;

TEST_F(PrintOptionValueTest, PrintOptionDefaultValue) {
std::string Output =
runTest(cl::init(OptionValue::Val),
cl::values(clEnumValN(OptionValue::Val, "v1", "desc1")));

EXPECT_EQ(Output, (" --" + Opt + " = v1 (default: v1)\n").str());
}

TEST_F(PrintOptionValueTest, PrintOptionNoDefaultValue) {
std::string Output =
runTest(cl::values(clEnumValN(OptionValue::Val, "v1", "desc1")));

// Note: the option still has a (zero-initialized) value, but the default
// is invalid and doesn't match any value.
EXPECT_EQ(Output, (" --" + Opt + " = v1 (default: )\n").str());
}

TEST_F(PrintOptionValueTest, PrintOptionUnknownValue) {
std::string Output = runTest(cl::init(OptionValue::Val));

EXPECT_EQ(Output, (" --" + Opt + " = *unknown option value*\n").str());
}

class GetOptionWidthTest : public ::testing::Test {
public:
enum class OptionValue { Val };
Expand Down
12 changes: 7 additions & 5 deletions mlir/test/Pass/pipeline-options-parsing.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,18 @@
// RUN: not mlir-opt %s -pass-pipeline='builtin.module(builtin.module(test-module-pass{test-option=3}))' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_2 %s
// RUN: not mlir-opt %s -pass-pipeline='builtin.module(builtin.module(func.func(test-options-pass{list=3}), test-module-pass{invalid-option=3}))' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_3 %s
// RUN: not mlir-opt %s -pass-pipeline='builtin.module(test-options-pass{list=3 list=notaninteger})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_4 %s
// RUN: not mlir-opt %s -pass-pipeline='builtin.module(test-options-pass{enum=invalid})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_5 %s
// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(test-options-pass{list=1,2,3,4 list=5 string=value1 string=value2}))'
// RUN: mlir-opt %s -verify-each=false -pass-pipeline='builtin.module(func.func(test-options-pass{string-list=a list=1,2,3,4 string-list=b,c list=5 string-list=d string=nested_pipeline{arg1=10 arg2=" {} " arg3=true}}))' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_1 %s
// RUN: mlir-opt %s -verify-each=false -test-options-pass-pipeline='list=1 string-list=a,b' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_2 %s
// RUN: mlir-opt %s -verify-each=false -pass-pipeline='builtin.module(builtin.module(func.func(test-options-pass{list=3}), func.func(test-options-pass{list=1,2,3,4})))' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_3 %s
// RUN: mlir-opt %s -verify-each=false -test-options-pass-pipeline='list=1 string-list=a,b enum=one' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_2 %s
// RUN: mlir-opt %s -verify-each=false -pass-pipeline='builtin.module(builtin.module(func.func(test-options-pass{list=3}), func.func(test-options-pass{enum=one list=1,2,3,4})))' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_3 %s

// CHECK_ERROR_1: missing closing '}' while processing pass options
// CHECK_ERROR_2: no such option test-option
// CHECK_ERROR_3: no such option invalid-option
// CHECK_ERROR_4: 'notaninteger' value invalid for integer argument
// CHECK_ERROR_5: for the --enum option: Cannot find option named 'invalid'!

// CHECK_1: test-options-pass{list=1,2,3,4,5 string=nested_pipeline{arg1=10 arg2=" {} " arg3=true} string-list=a,b,c,d}
// CHECK_2: test-options-pass{list=1 string= string-list=a,b}
// CHECK_3: builtin.module(builtin.module(func.func(test-options-pass{list=3 string= }),func.func(test-options-pass{list=1,2,3,4 string= })))
// CHECK_1: test-options-pass{enum=zero list=1,2,3,4,5 string=nested_pipeline{arg1=10 arg2=" {} " arg3=true} string-list=a,b,c,d}
// CHECK_2: test-options-pass{enum=one list=1 string= string-list=a,b}
// CHECK_3: builtin.module(builtin.module(func.func(test-options-pass{enum=zero list=3 string= }),func.func(test-options-pass{enum=one list=1,2,3,4 string= })))
11 changes: 11 additions & 0 deletions mlir/test/lib/Pass/TestPassManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,20 +53,27 @@ struct TestOptionsPass
: public PassWrapper<TestOptionsPass, OperationPass<func::FuncOp>> {
MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestOptionsPass)

enum Enum {};

struct Options : public PassPipelineOptions<Options> {
ListOption<int> listOption{*this, "list",
llvm::cl::desc("Example list option")};
ListOption<std::string> stringListOption{
*this, "string-list", llvm::cl::desc("Example string list option")};
Option<std::string> stringOption{*this, "string",
llvm::cl::desc("Example string option")};
Option<Enum> enumOption{
*this, "enum", llvm::cl::desc("Example enum option"),
llvm::cl::values(clEnumValN(0, "zero", "Example zero value"),
clEnumValN(1, "one", "Example one value"))};
};
TestOptionsPass() = default;
TestOptionsPass(const TestOptionsPass &) : PassWrapper() {}
TestOptionsPass(const Options &options) {
listOption = options.listOption;
stringOption = options.stringOption;
stringListOption = options.stringListOption;
enumOption = options.enumOption;
}

void runOnOperation() final {}
Expand All @@ -81,6 +88,10 @@ struct TestOptionsPass
*this, "string-list", llvm::cl::desc("Example string list option")};
Option<std::string> stringOption{*this, "string",
llvm::cl::desc("Example string option")};
Option<Enum> enumOption{
*this, "enum", llvm::cl::desc("Example enum option"),
llvm::cl::values(clEnumValN(0, "zero", "Example zero value"),
clEnumValN(1, "one", "Example one value"))};
};

/// A test pass that always aborts to enable testing the crash recovery
Expand Down