-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[llvm] Add subcommand support for OptTable #155026
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
Conversation
TODO: Add tests.
HelloSubOptTable T; | ||
unsigned MissingArgIndex, MissingArgCount; | ||
|
||
auto HandleMultipleSubcommands = [](const ArrayRef<StringRef> SubCommands) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
might not be needed since ArrayRef
is already immutable I believe. Same with the other const ArrayRef
s below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL.
// The option CommandIDsOffset. | ||
OS << ", "; | ||
if (R.getValue("CommandGroup") != nullptr) { | ||
std::vector<const Record *> CommandGroup = | ||
R.getValueAsListOfDefs("CommandGroup"); | ||
CommandKeyT CommandKey; | ||
for (const auto &Command : CommandGroup) | ||
CommandKey.push_back(Command->getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this into its own function since it's reused below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL.
for (auto SC : SubCommands) { | ||
llvm::errs() << " `" << SC << "`\n"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omit braces for single-line bodies: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
Same for other bodies below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL.
llvm/include/llvm/Option/OptTable.h
Outdated
OptTable(const StringTable &StrTable, | ||
ArrayRef<StringTable::Offset> PrefixesTable, | ||
ArrayRef<Info> OptionInfos, bool IgnoreCase = false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably just add
ArrayRef<Command> Commands = {},
ArrayRef<unsigned> CommandIDsTable = {},
so you don't need to make another ctor. Same with other OpTables below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL.
llvm/lib/Option/ArgList.cpp
Outdated
} | ||
if (SubCommands.size() == 1) | ||
return SubCommands.front(); | ||
return SubCommand; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SubCommand
is still just {}
here so maybe just return {}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL.
std::function<void(ArrayRef<StringRef>)> HandleMultipleSubcommands, | ||
std::function<void(ArrayRef<StringRef>)> HandleOtherPositionals) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add comments above briefly explaining what these callbacks are for. Mostly requesting since the implication to me (based on the example usage) is that a user might want to just throw an error and exit. If this is the primary/only desirable case, then that might be worth noting. I personally can't think of a reason one might not want to print help and exit, but maybe someone else might have a reason to. We could probably discourage that with comments or maybe saying the callback is NoReturn if we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL.
llvm/lib/Option/OptTable.cpp
Outdated
for (const auto &C : Commands) { | ||
if (FirstArg == C.Name) { | ||
ActiveCommand = &C; | ||
break; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Just a personal preference so feel free to ignore, but I think this looks cleaner
auto found = std::find_if(Commands.begin(), Commands.end(), [&](const auto &C){
return FirstArg == C.Name
});
ActiveCommand = found == Commands.end() ? nullptr : *found;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, it looks like you have getActiveCommand
below so maybe you could reuse it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reusing getActiveCommand. Using find_if in getActiveCommand implementation.
Done. PTAL.
llvm/lib/Option/OptTable.cpp
Outdated
// This loop prints subcommands list and sets ActiveCommand to | ||
// TopLevelCommand while iterating over all commands. | ||
for (const auto &C : Commands) { | ||
if (C.Name == TopLevelCommandName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coud probably just do C.Name == "TopLevelCommandName"
if it's not changed elsewhere and stick it as a constexpr
global in OptTable.h
if it's used elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: result of comparison against a string literal is unspecified (use an explicit string comparison function instead) [-Wstring-compare]
804 | if (C.Name == "TopLevelCommand") {
| ^ ~~~~~~~~~~~~~~~~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL.
assert((CurIndex == 0 || !Command.empty()) && | ||
"Only first command set should be empty!"); | ||
for (const auto &CommandKey : Command) { | ||
auto It = llvm::find_if(Commands, [&](const Record *R) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use std::find_if
directly. I think llvm::find_if
just calls it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL.
@PiJoules -- Thank you for the review! I've addressed all the comments from the last version you reviewed. PTAL. |
class HelloSubOptTable : public GenericOptTable { | ||
public: | ||
HelloSubOptTable() | ||
: GenericOptTable(OptionStrTable, OptionPrefixesTable, InfoTable, false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://llvm.org/docs/CodingStandards.html#comment-formatting
/*IgnoreCase=*/false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thank you.
if (Subcommand.empty()) { | ||
if (Args.hasArg(OPT_version)) { | ||
llvm::outs() << "LLVM Hello Subcommand Example 1.0\n"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (Args.hasArg(OPT_version))
llvm::outs() << "LLVM Hello Subcommand Example 1.0\n";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thank you.
llvm/lib/Option/ArgList.cpp
Outdated
|
||
size_t OldSize = SubCommands.size(); | ||
for (const OptTable::Command &CMD : Commands) { | ||
if (StringRef(CMD.Name) == "TopLevelCommand") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe extract "TopLevelCommand"
into a header and make it a global rather than using it as a literal between different cpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thank you. It's added to OptTable.h file
// Compile time representation for top level command (aka toolname). | ||
// Offers backward compatibility with existing Option class definitions before | ||
// introduction of commandGroup in Option class to support subcommands. | ||
def TopLevelCommand : Command<"TopLevelCommand">; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a user would define a Subcommand
with the name "TopLevelCommand"
? I'm assuming there should only be one with this name assuming it's like a sentinel. If only one should be defined, are there any checks that would assert this or diagnose if more than one was provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I'll add the check. Maybe it's worth changing the name to something that is probably not going to be useful as a user defined subcommand name. "TopLevelCommand" -> "TOPLEVELCOMMAND". Other option could be a "Command" type object with empty string for name to depict the top level command. I can make sure all the subcommands have a non empty string name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I did a refactoring which changes the design a bit. There is no abstraction for "TopLevelCommand" anymore which I always felt weird about maintaining. Now we only have subcommands that can be registered by the user. All regression tests pass and my llvm-hello-sub example behaves correctly. PTAL. I am working on adding more tests to cover subcommand use cases.
llvm/lib/Option/OptTable.cpp
Outdated
return internalParseOneArg(Args, Index, [VisibilityMask](const Option &Opt) { | ||
return !Opt.hasVisibilityFlag(VisibilityMask); | ||
}); | ||
return internalParseOneArg(Args, Index, nullptr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/*ActiveCommand=*/nullptr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thank you.
llvm/lib/Option/OptTable.cpp
Outdated
unsigned FlagsToExclude) const { | ||
return internalParseOneArg( | ||
Args, Index, [FlagsToInclude, FlagsToExclude](const Option &Opt) { | ||
Args, Index, nullptr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thank you.
llvm/lib/Option/OptTable.cpp
Outdated
FlagsToExclude &= ~HelpHidden; | ||
return internalPrintHelp( | ||
OS, Usage, Title, ShowHidden, ShowAllAliases, | ||
OS, Usage, Title, {}, ShowHidden, ShowAllAliases, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/*Subcommand=*/{}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thank you.
llvm/include/llvm/Option/OptTable.h
Outdated
#define LLVM_OPTION_OPTTABLE_H | ||
|
||
#include "llvm/ADT/ArrayRef.h" | ||
#include "llvm/ADT/SmallSet.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smallset seems unused here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
llvm/lib/Option/ArgList.cpp
Outdated
#include "llvm/Option/Option.h" | ||
#include "llvm/Support/Compiler.h" | ||
#include "llvm/Support/Debug.h" | ||
#include "llvm/Support/Error.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error seems unused here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
llvm/lib/Option/OptTable.cpp
Outdated
|
||
MissingArgIndex = MissingArgCount = 0; | ||
unsigned Index = 0, End = ArgArr.size(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably exclude the newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
llvm/lib/Option/OptTable.cpp
Outdated
std::unique_ptr<Arg> A = | ||
GroupedShortOptions ? parseOneArgGrouped(Args, Index) | ||
: internalParseOneArg(Args, Index, ExcludeOption); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably can exclude this formatting change
llvm/lib/Option/OptTable.cpp
Outdated
if (ID == ActiveSubCommandID) { | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single line if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return std::find(SubCommandIDs.begin(), SubCommandIDs.end(), ActiveSubCommandID) != SubCommandIDs.end();
might be cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Thanks. Done.
llvm/lib/Option/OptTable.cpp
Outdated
const SubCommand *ActiveSubCommand = | ||
std::find_if(SubCommands.begin(), SubCommands.end(), | ||
[&](const auto &C) { return Subcommand == C.Name; }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be misremembering, but find_if should return an iterator right?
Also is it always guaranteed here that ActiveSubCommand
will be nonnull?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Updated the LHS to const auto. ActiveSubCommand could be null. There is an assert check to verify its not where we have "SubCommand" StringRef is not empty. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the return type to be the iterator type and updated the assert check accordingly. PTAL.
llvm/lib/Option/OptTable.cpp
Outdated
continue; | ||
|
||
const Info &CandidateInfo = getInfo(Id); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
for (auto SC : SubCommands) { | ||
RSO1 << "\n" << SC; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single line loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
for (auto SC : Positionals) { | ||
RSO1 << "\n" << SC; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
EXPECT_TRUE(AL.hasArg(OPT_uppercase)); | ||
EXPECT_FALSE(AL.hasArg(OPT_lowercase)); | ||
EXPECT_FALSE(AL.hasArg(OPT_version)); | ||
// Do not expect any error messages as this is a valid use case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're using gtest, it might be more verbose if the comment was part of the error message on an expect failure, so something like:
EXPECT_EQ(std::string::npos, ErrMsg.find("Multiple subcommands passed")) << "Do not expect any error messages as this is a valid use case.";
same might be good for other comments above individual EXPECT lines below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thank you.
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks
llvm/include/llvm/Option/OptTable.h
Outdated
StringRef SubCommand) const { | ||
assert(!SubCommand.empty() && | ||
"This helper is only for valid registered subcommands."); | ||
typename ArrayRef<OptTable::SubCommand>::iterator SCIT = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can probably save a line and just use auto
here since it's probably well known that std::find*
functions return an iterator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. But it didn't save a line unfortunately :)
llvm/include/llvm/Option/OptTable.h
Outdated
/// Return the string table used for option names. | ||
const StringTable &getStrTable() const { return *StrTable; } | ||
|
||
const ArrayRef<SubCommand> getSubCommands() const { return SubCommands; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArrayRef
is already immutable so const
might not be needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
llvm/lib/Option/OptTable.cpp
Outdated
// pairs. | ||
std::map<std::string, std::vector<OptionInfo>> GroupedOptionHelp; | ||
|
||
typename ArrayRef<OptTable::SubCommand>::iterator ActiveSubCommand = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can probably use auto
here also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Thank you @PiJoules I'll wait for a day to see if there are any objections before landing. |
this is for unblocking eld upstream : llvm/llvm-project#155026 Signed-off-by: Shankar Easwaran <[email protected]>
this is for unblocking eld upstream : llvm/llvm-project#155026 Signed-off-by: Shankar Easwaran <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having a directory in examples
dedicated to just subcommands, I think we should a directory for Option
and subcommands should be just one of the examples in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll start a new PR for that change.
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/14465 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/16524 Here is the relevant piece of the build log for the reference
|
Implement support for
subcommands
in OptTable to attain feature parity withcl
.Design overview: https://discourse.llvm.org/t/subcommand-feature-support-in-llvm-opttable/88098
Issue: #108307