From 444a635e530b72bc3ca40dd0984c1d4c8c5da0a3 Mon Sep 17 00:00:00 2001 From: Mingming Liu Date: Wed, 8 Nov 2023 21:34:04 -0800 Subject: [PATCH 1/5] [Support]Look up in top-level subcommand as a fallback when looking options for a custom subcommand. Context: In https://lists.llvm.org/pipermail/llvm-dev/2016-June/101804.html and commit 07670b3e984db32f291373fe12c392959f2aff67, cl::SubCommand is introduced. Options that don't specify subcommand goes intoa special 'top level' subcommand. Motivating Use Case: The motivating use case is to refactor llvm-profdata to use cl::SubCommand to organize subcommands. See pr/71328. A valid use case that's not supported before this patch // show-option{1,2} are associated with 'show' subcommand. // top-level-option3 is in top-level subcomand (e.g., `profile-isfs` in SampleProfReader.cpp) llvm-profdata show --show-option1 --show-option2 --top-level-option3 - Before this patch, option handler look-up will fail with the following error message "Unknown command line argument --top-level-option3". - After this patch, option handler look-up will look up in sub-command options first, and use top-level subcommand as a fallback, so 'top-level-option3' is parsed correctly. --- llvm/lib/Support/CommandLine.cpp | 7 +++ llvm/unittests/Support/CommandLineTest.cpp | 68 ++++++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/llvm/lib/Support/CommandLine.cpp b/llvm/lib/Support/CommandLine.cpp index 55633d7cafa47..a7e0cae8b855d 100644 --- a/llvm/lib/Support/CommandLine.cpp +++ b/llvm/lib/Support/CommandLine.cpp @@ -1667,6 +1667,13 @@ bool CommandLineParser::ParseCommandLineOptions(int argc, Handler = LookupLongOption(*ChosenSubCommand, ArgName, Value, LongOptionsUseDoubleDash, HaveDoubleDash); + // If Handler is not found in a specialized subcommand, look up handler + // in the top-level subcommand. + // cl::opt without cl::sub belongs to top-level subcommand. + if (!Handler && ChosenSubCommand != &SubCommand::getTopLevel()) + Handler = LookupLongOption(SubCommand::getTopLevel(), ArgName, Value, + LongOptionsUseDoubleDash, HaveDoubleDash); + // Check to see if this "option" is really a prefixed or grouped argument. if (!Handler && !(LongOptionsUseDoubleDash && HaveDoubleDash)) Handler = HandlePrefixedOrGroupedOption(ArgName, Value, ErrorParsing, diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp index 41cc8260acfed..76589c7deed88 100644 --- a/llvm/unittests/Support/CommandLineTest.cpp +++ b/llvm/unittests/Support/CommandLineTest.cpp @@ -525,6 +525,74 @@ TEST(CommandLineTest, LookupFailsInWrongSubCommand) { EXPECT_FALSE(Errs.empty()); } +TEST(CommandLineTest, SubcommandOptions) { + enum LiteralOptionEnum { + foo, + bar, + baz, + }; + + cl::ResetCommandLineParser(); + + // This is a top-level option and not associated with a subcommand. + // A command line using subcommand should parse both subcommand options and + // top-level options. A valid use case is that users of llvm command line + // tools should be able to specify top-level options defined in any library. + cl::opt TopLevelOpt("str", cl::init("txt"), + cl::desc("A top-level option.")); + + StackSubCommand SC("sc", "Subcommand"); + + // The positional argument. + StackOption PositionalOpt( + cl::Positional, cl::desc("positional argument test coverage"), + cl::sub(SC)); + // Thel literal argument. + StackOption LiteralOpt( + cl::desc("literal argument test coverage"), cl::sub(SC), cl::init(bar), + cl::values(clEnumVal(foo, "foo"), clEnumVal(bar, "bar"), + clEnumVal(baz, "baz"))); + StackOption BoolOpt("enable", cl::sub(SC), cl::init(false)); + + std::string Errs; + raw_string_ostream OS(Errs); + + for (const char *literalArg : {"--bar", "--foo", "--baz"}) { + const char *args[] = {"prog", "sc", "input-file", + literalArg, "-enable", "--str=csv"}; + + // cl::ParseCommandLineOptions returns true on success. it returns false + // and prints errors to caller provided error stream (&OS in this setting). + EXPECT_TRUE(cl::ParseCommandLineOptions(6, args, StringRef(), &OS)); + + // Tests that the value of `str` option is `csv` as specified. + EXPECT_EQ(strcmp(TopLevelOpt.getValue().c_str(), "csv"), 0); + + const char *parsedLiteralOpt; + switch (LiteralOpt) { + case baz: + parsedLiteralOpt = "baz"; + break; + case bar: + parsedLiteralOpt = "bar"; + break; + case foo: + parsedLiteralOpt = "foo"; + break; + default: + llvm_unreachable("unknown option for LiteralOpt"); + } + + // Tests that literal options are parsed correctly. Skip '--' prefix of + // literalArg. + EXPECT_EQ(strcmp(parsedLiteralOpt, literalArg + 2), 0); + + // Flush and tests there is no error message. + OS.flush(); + EXPECT_TRUE(Errs.empty()); + } +} + TEST(CommandLineTest, AddToAllSubCommands) { cl::ResetCommandLineParser(); From 707dcabfc7caa61617d894e5f76ae2e76f6bbf83 Mon Sep 17 00:00:00 2001 From: Mingming Liu Date: Thu, 9 Nov 2023 10:29:42 -0800 Subject: [PATCH 2/5] resolve feedback --- llvm/unittests/Support/CommandLineTest.cpp | 61 ++++++++-------------- 1 file changed, 23 insertions(+), 38 deletions(-) diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp index 76589c7deed88..b6d84c37b606c 100644 --- a/llvm/unittests/Support/CommandLineTest.cpp +++ b/llvm/unittests/Support/CommandLineTest.cpp @@ -542,54 +542,39 @@ TEST(CommandLineTest, SubcommandOptions) { cl::desc("A top-level option.")); StackSubCommand SC("sc", "Subcommand"); - // The positional argument. StackOption PositionalOpt( cl::Positional, cl::desc("positional argument test coverage"), cl::sub(SC)); - // Thel literal argument. + // The literal argument. StackOption LiteralOpt( cl::desc("literal argument test coverage"), cl::sub(SC), cl::init(bar), cl::values(clEnumVal(foo, "foo"), clEnumVal(bar, "bar"), clEnumVal(baz, "baz"))); StackOption BoolOpt("enable", cl::sub(SC), cl::init(false)); - std::string Errs; - raw_string_ostream OS(Errs); - - for (const char *literalArg : {"--bar", "--foo", "--baz"}) { - const char *args[] = {"prog", "sc", "input-file", - literalArg, "-enable", "--str=csv"}; - - // cl::ParseCommandLineOptions returns true on success. it returns false - // and prints errors to caller provided error stream (&OS in this setting). - EXPECT_TRUE(cl::ParseCommandLineOptions(6, args, StringRef(), &OS)); - - // Tests that the value of `str` option is `csv` as specified. - EXPECT_EQ(strcmp(TopLevelOpt.getValue().c_str(), "csv"), 0); - - const char *parsedLiteralOpt; - switch (LiteralOpt) { - case baz: - parsedLiteralOpt = "baz"; - break; - case bar: - parsedLiteralOpt = "bar"; - break; - case foo: - parsedLiteralOpt = "foo"; - break; - default: - llvm_unreachable("unknown option for LiteralOpt"); - } - - // Tests that literal options are parsed correctly. Skip '--' prefix of - // literalArg. - EXPECT_EQ(strcmp(parsedLiteralOpt, literalArg + 2), 0); - - // Flush and tests there is no error message. - OS.flush(); - EXPECT_TRUE(Errs.empty()); + const char *positionalOptVal = "input-file"; + const char *args[] = {"prog", "sc", positionalOptVal, "-enable", "--str=csv"}; + + // cl::ParseCommandLineOptions returns true on success. Otherwise, it will + // print the error message to stderr and exit in this setting (`Errs` ostream + // is not set). + ASSERT_TRUE(cl::ParseCommandLineOptions(sizeof(args) / sizeof(args[0]), args, + StringRef())); + EXPECT_STREQ(PositionalOpt.getValue().c_str(), positionalOptVal); + EXPECT_TRUE(BoolOpt); + // Tests that the value of `str` option is `csv` as specified. + EXPECT_STREQ(TopLevelOpt.getValue().c_str(), "csv"); + + for (auto &[literalOptVal, wantLiteralOpt] : + {std::make_pair("--bar", bar), std::make_pair("--foo", foo), + std::make_pair("--baz", baz)}) { + const char *args[] = {"prog", "sc", literalOptVal}; + ASSERT_TRUE(cl::ParseCommandLineOptions(sizeof(args) / sizeof(args[0]), + args, StringRef())); + + // Tests that literal options are parsed correctly. + EXPECT_EQ(LiteralOpt, wantLiteralOpt); } } From 5d90e446a9e10a5508c9f7e587f6c06cef6f4598 Mon Sep 17 00:00:00 2001 From: Mingming Liu Date: Thu, 9 Nov 2023 11:39:43 -0800 Subject: [PATCH 3/5] Integrate offline feedback. Use CamelCase variable name that starts with upper case letter; and use std::pair initializer. --- llvm/unittests/Support/CommandLineTest.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp index b6d84c37b606c..156c46a36d680 100644 --- a/llvm/unittests/Support/CommandLineTest.cpp +++ b/llvm/unittests/Support/CommandLineTest.cpp @@ -553,28 +553,28 @@ TEST(CommandLineTest, SubcommandOptions) { clEnumVal(baz, "baz"))); StackOption BoolOpt("enable", cl::sub(SC), cl::init(false)); - const char *positionalOptVal = "input-file"; - const char *args[] = {"prog", "sc", positionalOptVal, "-enable", "--str=csv"}; + const char *PositionalOptVal = "input-file"; + const char *args[] = {"prog", "sc", PositionalOptVal, "-enable", "--str=csv"}; // cl::ParseCommandLineOptions returns true on success. Otherwise, it will // print the error message to stderr and exit in this setting (`Errs` ostream // is not set). ASSERT_TRUE(cl::ParseCommandLineOptions(sizeof(args) / sizeof(args[0]), args, StringRef())); - EXPECT_STREQ(PositionalOpt.getValue().c_str(), positionalOptVal); + EXPECT_STREQ(PositionalOpt.getValue().c_str(), PositionalOptVal); EXPECT_TRUE(BoolOpt); // Tests that the value of `str` option is `csv` as specified. EXPECT_STREQ(TopLevelOpt.getValue().c_str(), "csv"); - for (auto &[literalOptVal, wantLiteralOpt] : - {std::make_pair("--bar", bar), std::make_pair("--foo", foo), - std::make_pair("--baz", baz)}) { - const char *args[] = {"prog", "sc", literalOptVal}; + for (auto &[LiteralOptVal, WantLiteralOpt] : + {std::pair{"--bar", bar}, std::pair{"--foo", foo}, + std::pair{"--baz", baz}}) { + const char *args[] = {"prog", "sc", LiteralOptVal}; ASSERT_TRUE(cl::ParseCommandLineOptions(sizeof(args) / sizeof(args[0]), args, StringRef())); // Tests that literal options are parsed correctly. - EXPECT_EQ(LiteralOpt, wantLiteralOpt); + EXPECT_EQ(LiteralOpt, WantLiteralOpt); } } From d76b4d9d1c949fcc984d552cfce27fe9015fcabd Mon Sep 17 00:00:00 2001 From: Mingming Liu Date: Thu, 9 Nov 2023 11:44:50 -0800 Subject: [PATCH 4/5] Per offline feedback, keep std::pair only for the first element --- llvm/unittests/Support/CommandLineTest.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp index 156c46a36d680..58e7fbbe9dea7 100644 --- a/llvm/unittests/Support/CommandLineTest.cpp +++ b/llvm/unittests/Support/CommandLineTest.cpp @@ -567,8 +567,7 @@ TEST(CommandLineTest, SubcommandOptions) { EXPECT_STREQ(TopLevelOpt.getValue().c_str(), "csv"); for (auto &[LiteralOptVal, WantLiteralOpt] : - {std::pair{"--bar", bar}, std::pair{"--foo", foo}, - std::pair{"--baz", baz}}) { + {std::pair{"--bar", bar}, {"--foo", foo}, {"--baz", baz}}) { const char *args[] = {"prog", "sc", LiteralOptVal}; ASSERT_TRUE(cl::ParseCommandLineOptions(sizeof(args) / sizeof(args[0]), args, StringRef())); From 40840d312cf7d02f26c8c729869f689c042c7e49 Mon Sep 17 00:00:00 2001 From: Mingming Liu Date: Thu, 9 Nov 2023 23:24:02 -0800 Subject: [PATCH 5/5] resolve feedback --- llvm/unittests/Support/CommandLineTest.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp index 58e7fbbe9dea7..4411ed0f83928 100644 --- a/llvm/unittests/Support/CommandLineTest.cpp +++ b/llvm/unittests/Support/CommandLineTest.cpp @@ -525,7 +525,7 @@ TEST(CommandLineTest, LookupFailsInWrongSubCommand) { EXPECT_FALSE(Errs.empty()); } -TEST(CommandLineTest, SubcommandOptions) { +TEST(CommandLineTest, TopLevelOptInSubcommand) { enum LiteralOptionEnum { foo, bar, @@ -542,19 +542,19 @@ TEST(CommandLineTest, SubcommandOptions) { cl::desc("A top-level option.")); StackSubCommand SC("sc", "Subcommand"); - // The positional argument. StackOption PositionalOpt( cl::Positional, cl::desc("positional argument test coverage"), cl::sub(SC)); - // The literal argument. StackOption LiteralOpt( cl::desc("literal argument test coverage"), cl::sub(SC), cl::init(bar), cl::values(clEnumVal(foo, "foo"), clEnumVal(bar, "bar"), clEnumVal(baz, "baz"))); - StackOption BoolOpt("enable", cl::sub(SC), cl::init(false)); + StackOption EnableOpt("enable", cl::sub(SC), cl::init(false)); + StackOption ThresholdOpt("threshold", cl::sub(SC), cl::init(1)); const char *PositionalOptVal = "input-file"; - const char *args[] = {"prog", "sc", PositionalOptVal, "-enable", "--str=csv"}; + const char *args[] = {"prog", "sc", PositionalOptVal, + "-enable", "--str=csv", "--threshold=2"}; // cl::ParseCommandLineOptions returns true on success. Otherwise, it will // print the error message to stderr and exit in this setting (`Errs` ostream @@ -562,9 +562,10 @@ TEST(CommandLineTest, SubcommandOptions) { ASSERT_TRUE(cl::ParseCommandLineOptions(sizeof(args) / sizeof(args[0]), args, StringRef())); EXPECT_STREQ(PositionalOpt.getValue().c_str(), PositionalOptVal); - EXPECT_TRUE(BoolOpt); + EXPECT_TRUE(EnableOpt); // Tests that the value of `str` option is `csv` as specified. EXPECT_STREQ(TopLevelOpt.getValue().c_str(), "csv"); + EXPECT_EQ(ThresholdOpt, 2); for (auto &[LiteralOptVal, WantLiteralOpt] : {std::pair{"--bar", bar}, {"--foo", foo}, {"--baz", baz}}) {