[llvm] Fix logic to detect cl::option equality. (PR #65754)
Christian Sigg via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 8 06:19:58 PDT 2023
https://github.com/chsigg created https://github.com/llvm/llvm-project/pull/65754:
This is a new attempt of https://reviews.llvm.org/D159481, this time as GitHub PR.
`GenericOptionValue::compare()` should return `true` for a match.
- `OptionValueBase::compare()` always returns `false` and shouldn't match anything.
- `OptionValueCopy::compare()` returns `false` if not `Valid` which corresponds to no match.
Also adding some tests.
>From 3434fb02d09112abe8b7d3fc48f91f3156748cde Mon Sep 17 00:00:00 2001
From: Christian Sigg <csigg at google.com>
Date: Fri, 8 Sep 2023 15:11:42 +0200
Subject: [PATCH] Fix logic to detect option string/value equality.
`GenericOptionValue::compare()` should return `true` for a match.
---
llvm/include/llvm/Support/CommandLine.h | 6 ++-
llvm/lib/Support/CommandLine.cpp | 4 +-
llvm/unittests/Support/CommandLineTest.cpp | 49 ++++++++++++++++----
mlir/test/Pass/pipeline-options-parsing.mlir | 12 +++--
mlir/test/lib/Pass/TestPassManager.cpp | 11 +++++
5 files changed, 64 insertions(+), 18 deletions(-)
diff --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h
index d2079fead66808c..49f4a668ae416fe 100644
--- a/llvm/include/llvm/Support/CommandLine.h
+++ b/llvm/include/llvm/Support/CommandLine.h
@@ -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 {
@@ -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 =
@@ -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);
}
diff --git a/llvm/lib/Support/CommandLine.cpp b/llvm/lib/Support/CommandLine.cpp
index d3efb8b67be5c67..55633d7cafa4791 100644
--- a/llvm/lib/Support/CommandLine.cpp
+++ b/llvm/lib/Support/CommandLine.cpp
@@ -2181,7 +2181,7 @@ 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);
@@ -2189,7 +2189,7 @@ void generic_parser_base::printGenericOptionDiff(
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;
diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp
index 4f7cb40263b37fd..41cc8260acfedf7 100644
--- a/llvm/unittests/Support/CommandLineTest.cpp
+++ b/llvm/unittests/Support/CommandLineTest.cpp
@@ -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.
@@ -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);
@@ -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 =
@@ -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
@@ -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 };
diff --git a/mlir/test/Pass/pipeline-options-parsing.mlir b/mlir/test/Pass/pipeline-options-parsing.mlir
index 33bef75ee94a2b0..34313cd55660333 100644
--- a/mlir/test/Pass/pipeline-options-parsing.mlir
+++ b/mlir/test/Pass/pipeline-options-parsing.mlir
@@ -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= })))
diff --git a/mlir/test/lib/Pass/TestPassManager.cpp b/mlir/test/lib/Pass/TestPassManager.cpp
index b9cd217839bb9c8..3b24cf44e211080 100644
--- a/mlir/test/lib/Pass/TestPassManager.cpp
+++ b/mlir/test/lib/Pass/TestPassManager.cpp
@@ -53,6 +53,8 @@ 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")};
@@ -60,6 +62,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"))};
};
TestOptionsPass() = default;
TestOptionsPass(const TestOptionsPass &) : PassWrapper() {}
@@ -67,6 +73,7 @@ struct TestOptionsPass
listOption = options.listOption;
stringOption = options.stringOption;
stringListOption = options.stringListOption;
+ enumOption = options.enumOption;
}
void runOnOperation() final {}
@@ -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
More information about the llvm-commits
mailing list