Skip to content

Commit 710b5a1

Browse files
chsiggchsigg
authored
Fix logic to detect cl::option equality. (#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.
1 parent 18b6e21 commit 710b5a1

File tree

5 files changed

+64
-18
lines changed

5 files changed

+64
-18
lines changed

‎llvm/include/llvm/Support/CommandLine.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,7 @@ struct OptionValueBase : public GenericOptionValue {
552552
// Some options may take their value from a different data type.
553553
template <class DT> void setValue(const DT & /*V*/) {}
554554

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

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

590-
bool compare(const DataType &V) const { return Valid && (Value != V); }
591+
// Returns whether this instance matches V.
592+
bool compare(const DataType &V) const { return Valid && (Value == V); }
591593

592594
bool compare(const GenericOptionValue &V) const override {
593595
const OptionValueCopy<DataType> &VC =
@@ -1442,7 +1444,7 @@ class opt
14421444
}
14431445

14441446
void printOptionValue(size_t GlobalWidth, bool Force) const override {
1445-
if (Force || this->getDefault().compare(this->getValue())) {
1447+
if (Force || !this->getDefault().compare(this->getValue())) {
14461448
cl::printOptionDiff<ParserClass>(*this, Parser, this->getValue(),
14471449
this->getDefault(), GlobalWidth);
14481450
}

‎llvm/lib/Support/CommandLine.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2181,15 +2181,15 @@ void generic_parser_base::printGenericOptionDiff(
21812181

21822182
unsigned NumOpts = getNumOptions();
21832183
for (unsigned i = 0; i != NumOpts; ++i) {
2184-
if (Value.compare(getOptionValue(i)))
2184+
if (!Value.compare(getOptionValue(i)))
21852185
continue;
21862186

21872187
outs() << "= " << getOption(i);
21882188
size_t L = getOption(i).size();
21892189
size_t NumSpaces = MaxOptWidth > L ? MaxOptWidth - L : 0;
21902190
outs().indent(NumSpaces) << " (default: ";
21912191
for (unsigned j = 0; j != NumOpts; ++j) {
2192-
if (Default.compare(getOptionValue(j)))
2192+
if (!Default.compare(getOptionValue(j)))
21932193
continue;
21942194
outs() << getOption(j);
21952195
break;

‎llvm/unittests/Support/CommandLineTest.cpp‎

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1294,7 +1294,8 @@ struct AutoDeleteFile {
12941294
}
12951295
};
12961296

1297-
class PrintOptionInfoTest : public ::testing::Test {
1297+
template <void (*Func)(const cl::Option &)>
1298+
class PrintOptionTestBase : public ::testing::Test {
12981299
public:
12991300
// Return std::string because the output of a failing EXPECT check is
13001301
// unreadable for StringRef. It also avoids any lifetime issues.
@@ -1309,7 +1310,7 @@ class PrintOptionInfoTest : public ::testing::Test {
13091310

13101311
StackOption<OptionValue> TestOption(Opt, cl::desc(HelpText),
13111312
OptionAttributes...);
1312-
printOptionInfo(TestOption, 26);
1313+
Func(TestOption);
13131314
outs().flush();
13141315
}
13151316
auto Buffer = MemoryBuffer::getFile(File.FilePath);
@@ -1321,14 +1322,15 @@ class PrintOptionInfoTest : public ::testing::Test {
13211322
enum class OptionValue { Val };
13221323
const StringRef Opt = "some-option";
13231324
const StringRef HelpText = "some help";
1325+
};
13241326

1325-
private:
13261327
// This is a workaround for cl::Option sub-classes having their
13271328
// printOptionInfo functions private.
1328-
void printOptionInfo(const cl::Option &O, size_t Width) {
1329-
O.printOptionInfo(Width);
1330-
}
1331-
};
1329+
void printOptionInfo(const cl::Option &O) {
1330+
O.printOptionInfo(/*GlobalWidth=*/26);
1331+
}
1332+
1333+
using PrintOptionInfoTest = PrintOptionTestBase<printOptionInfo>;
13321334

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

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

1421+
void printOptionValue(const cl::Option &O) {
1422+
O.printOptionValue(/*GlobalWidth=*/12, /*Force=*/true);
1423+
}
1424+
1425+
using PrintOptionValueTest = PrintOptionTestBase<printOptionValue>;
1426+
1427+
TEST_F(PrintOptionValueTest, PrintOptionDefaultValue) {
1428+
std::string Output =
1429+
runTest(cl::init(OptionValue::Val),
1430+
cl::values(clEnumValN(OptionValue::Val, "v1", "desc1")));
1431+
1432+
EXPECT_EQ(Output, (" --" + Opt + " = v1 (default: v1)\n").str());
1433+
}
1434+
1435+
TEST_F(PrintOptionValueTest, PrintOptionNoDefaultValue) {
1436+
std::string Output =
1437+
runTest(cl::values(clEnumValN(OptionValue::Val, "v1", "desc1")));
1438+
1439+
// Note: the option still has a (zero-initialized) value, but the default
1440+
// is invalid and doesn't match any value.
1441+
EXPECT_EQ(Output, (" --" + Opt + " = v1 (default: )\n").str());
1442+
}
1443+
1444+
TEST_F(PrintOptionValueTest, PrintOptionUnknownValue) {
1445+
std::string Output = runTest(cl::init(OptionValue::Val));
1446+
1447+
EXPECT_EQ(Output, (" --" + Opt + " = *unknown option value*\n").str());
1448+
}
1449+
14191450
class GetOptionWidthTest : public ::testing::Test {
14201451
public:
14211452
enum class OptionValue { Val };

‎mlir/test/Pass/pipeline-options-parsing.mlir‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,18 @@
22
// 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
33
// 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
44
// 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
5+
// RUN: not mlir-opt %s -pass-pipeline='builtin.module(test-options-pass{enum=invalid})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_5 %s
56
// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(test-options-pass{list=1,2,3,4 list=5 string=value1 string=value2}))'
67
// 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
7-
// 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
8-
// 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
8+
// 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
9+
// 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
910

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

15-
// 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}
16-
// CHECK_2: test-options-pass{list=1 string= string-list=a,b}
17-
// 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= })))
17+
// 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}
18+
// CHECK_2: test-options-pass{enum=one list=1 string= string-list=a,b}
19+
// 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= })))

‎mlir/test/lib/Pass/TestPassManager.cpp‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,20 +53,27 @@ struct TestOptionsPass
5353
: public PassWrapper<TestOptionsPass, OperationPass<func::FuncOp>> {
5454
MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestOptionsPass)
5555

56+
enum Enum {};
57+
5658
struct Options : public PassPipelineOptions<Options> {
5759
ListOption<int> listOption{*this, "list",
5860
llvm::cl::desc("Example list option")};
5961
ListOption<std::string> stringListOption{
6062
*this, "string-list", llvm::cl::desc("Example string list option")};
6163
Option<std::string> stringOption{*this, "string",
6264
llvm::cl::desc("Example string option")};
65+
Option<Enum> enumOption{
66+
*this, "enum", llvm::cl::desc("Example enum option"),
67+
llvm::cl::values(clEnumValN(0, "zero", "Example zero value"),
68+
clEnumValN(1, "one", "Example one value"))};
6369
};
6470
TestOptionsPass() = default;
6571
TestOptionsPass(const TestOptionsPass &) : PassWrapper() {}
6672
TestOptionsPass(const Options &options) {
6773
listOption = options.listOption;
6874
stringOption = options.stringOption;
6975
stringListOption = options.stringListOption;
76+
enumOption = options.enumOption;
7077
}
7178

7279
void runOnOperation() final {}
@@ -81,6 +88,10 @@ struct TestOptionsPass
8188
*this, "string-list", llvm::cl::desc("Example string list option")};
8289
Option<std::string> stringOption{*this, "string",
8390
llvm::cl::desc("Example string option")};
91+
Option<Enum> enumOption{
92+
*this, "enum", llvm::cl::desc("Example enum option"),
93+
llvm::cl::values(clEnumValN(0, "zero", "Example zero value"),
94+
clEnumValN(1, "one", "Example one value"))};
8495
};
8596

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

0 commit comments

Comments
 (0)