Skip to content
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.

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

[Support]Look up in top-level subcommand as a fallback when looking options for a custom subcommand. #71776

Merged
merged 5 commits into from
Nov 10, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 7 additions & 0 deletions llvm/lib/Support/CommandLine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
68 changes: 68 additions & 0 deletions llvm/unittests/Support/CommandLineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,74 @@ TEST(CommandLineTest, LookupFailsInWrongSubCommand) {
EXPECT_FALSE(Errs.empty());
}

TEST(CommandLineTest, SubcommandOptions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other Subcommand tests. The name SubcommandOptions can be changed to reflect that the test intends to check, e.g. TopLevelOptInSubcommand

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code self explains. I think the comments are unneeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete two one-line comments.

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"}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you structure this loop to iterate over a list of {input, want} pairs then you can avoid the switch case below and the conversion for checking.

for (auto& test : {std::pair{"--bar", bar}, {"--foo", foo}, {"--baz", baz}) {
  ...
  EXPECT_EQ(LiteralOpt, test.second);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, done. Also move the loop-invariant test case out of the loop as a part of this commit.

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: sizeof(args) instead of 6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be ASSERT_TRUE since it does not make sense to continue if we failed to parse this arg. Though note that it will fail the entire test and the rest of the tests will not be executed (which imo is fine).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep ASSERT_TRUE is better in this context. done.


// Tests that the value of `str` option is `csv` as specified.
EXPECT_EQ(strcmp(TopLevelOpt.getValue().c_str(), "csv"), 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EXPECT_STREQ instead of EXPECT_EQ to avoid the strcmp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.


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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is unnecessary since we already check the return value of cl::ParseCommandLineOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I removed Errs and OS in this test case.

}
}

TEST(CommandLineTest, AddToAllSubCommands) {
cl::ResetCommandLineParser();

Expand Down