[llvm] [Support]Look up in top-level subcommand as a fallback when looking options for a custom subcommand. (PR #71776)
Mingming Liu via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 9 11:41:09 PST 2023
https://github.com/minglotus-6 updated https://github.com/llvm/llvm-project/pull/71776
>From 444a635e530b72bc3ca40dd0984c1d4c8c5da0a3 Mon Sep 17 00:00:00 2001
From: Mingming Liu <mingmingl at google.com>
Date: Wed, 8 Nov 2023 21:34:04 -0800
Subject: [PATCH 1/3] [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 55633d7cafa4791..a7e0cae8b855d7c 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 41cc8260acfedf7..76589c7deed88e1 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<std::string> TopLevelOpt("str", cl::init("txt"),
+ cl::desc("A top-level option."));
+
+ StackSubCommand SC("sc", "Subcommand");
+
+ // The positional argument.
+ StackOption<std::string> PositionalOpt(
+ cl::Positional, cl::desc("positional argument test coverage"),
+ cl::sub(SC));
+ // Thel literal argument.
+ StackOption<LiteralOptionEnum> 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<bool> 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 <mingmingl at google.com>
Date: Thu, 9 Nov 2023 10:29:42 -0800
Subject: [PATCH 2/3] 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 76589c7deed88e1..b6d84c37b606c6f 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<std::string> PositionalOpt(
cl::Positional, cl::desc("positional argument test coverage"),
cl::sub(SC));
- // Thel literal argument.
+ // The literal argument.
StackOption<LiteralOptionEnum> 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<bool> 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 <mingmingl at google.com>
Date: Thu, 9 Nov 2023 11:39:43 -0800
Subject: [PATCH 3/3] 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 b6d84c37b606c6f..156c46a36d6805a 100644
--- a/llvm/unittests/Support/CommandLineTest.cpp
+++ b/llvm/unittests/Support/CommandLineTest.cpp
@@ -553,28 +553,28 @@ TEST(CommandLineTest, SubcommandOptions) {
clEnumVal(baz, "baz")));
StackOption<bool> 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);
}
}
More information about the llvm-commits
mailing list