[llvm] r365742 - Revert Recommit "[CommandLine] Remove OptionCategory and SubCommand caches from the Option class."
Haojian Wu via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 11 01:54:29 PDT 2019
Author: hokein
Date: Thu Jul 11 01:54:28 2019
New Revision: 365742
URL: http://llvm.org/viewvc/llvm-project?rev=365742&view=rev
Log:
Revert Recommit "[CommandLine] Remove OptionCategory and SubCommand caches from the Option class."
This reverts r365675 (git commit 43d75f977853c3ec891a440c362b2df183a211b5)
The patch causes a crash in SupportTests (CommandLineTest.AliasesWithArguments).
Modified:
llvm/trunk/include/llvm/Support/CommandLine.h
llvm/trunk/lib/Support/CommandLine.cpp
llvm/trunk/unittests/Support/CommandLineTest.cpp
Modified: llvm/trunk/include/llvm/Support/CommandLine.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/CommandLine.h?rev=365742&r1=365741&r2=365742&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Support/CommandLine.h (original)
+++ llvm/trunk/include/llvm/Support/CommandLine.h Thu Jul 11 01:54:28 2019
@@ -187,27 +187,24 @@ enum MiscFlags { // Miscella
//
class OptionCategory {
private:
- StringRef Name = "General Category";
- StringRef Description;
+ StringRef const Name;
+ StringRef const Description;
void registerCategory();
public:
- OptionCategory(StringRef Name,
- StringRef Description = "")
+ OptionCategory(StringRef const Name,
+ StringRef const Description = "")
: Name(Name), Description(Description) {
registerCategory();
}
- OptionCategory() = default;
StringRef getName() const { return Name; }
StringRef getDescription() const { return Description; }
-
- SmallPtrSet<Option *, 16> MemberOptions;
};
// The general Option Category (used as default category).
-extern ManagedStatic<OptionCategory> GeneralCategory;
+extern OptionCategory GeneralCategory;
//===----------------------------------------------------------------------===//
// SubCommand class
@@ -286,12 +283,9 @@ public:
StringRef ArgStr; // The argument string itself (ex: "help", "o")
StringRef HelpStr; // The descriptive text message for -help
StringRef ValueStr; // String describing what the value of this option is
-
- // Return the set of OptionCategories that this Option belongs to.
- SmallPtrSet<OptionCategory *, 1> getCategories() const;
-
- // Return the set of SubCommands that this Option belongs to.
- SmallPtrSet<SubCommand *, 1> getSubCommands() const;
+ SmallVector<OptionCategory *, 1>
+ Categories; // The Categories this option belongs to
+ SmallPtrSet<SubCommand *, 1> Subs; // The subcommands this option belongs to.
inline enum NumOccurrencesFlag getNumOccurrencesFlag() const {
return (enum NumOccurrencesFlag)Occurrences;
@@ -323,6 +317,12 @@ public:
return getNumOccurrencesFlag() == cl::ConsumeAfter;
}
+ bool isInAllSubCommands() const {
+ return any_of(Subs, [](const SubCommand *SC) {
+ return SC == &*AllSubCommands;
+ });
+ }
+
//-------------------------------------------------------------------------===
// Accessor functions set by OptionModifiers
//
@@ -336,13 +336,16 @@ public:
void setMiscFlag(enum MiscFlags M) { Misc |= M; }
void setPosition(unsigned pos) { Position = pos; }
void addCategory(OptionCategory &C);
+ void addSubCommand(SubCommand &S) { Subs.insert(&S); }
protected:
explicit Option(enum NumOccurrencesFlag OccurrencesFlag,
enum OptionHidden Hidden)
: NumOccurrences(0), Occurrences(OccurrencesFlag), Value(0),
HiddenFlag(Hidden), Formatting(NormalFormatting), Misc(0),
- FullyInitialized(false), Position(0), AdditionalVals(0) {}
+ FullyInitialized(false), Position(0), AdditionalVals(0) {
+ Categories.push_back(&GeneralCategory);
+ }
inline void setNumAdditionalVals(unsigned n) { AdditionalVals = n; }
@@ -351,14 +354,7 @@ public:
// addArgument - Register this argument with the commandline system.
//
- virtual void addArgument(SubCommand &SC);
-
- // addArgument - Only called in done() method to add default
- // TopLevelSubCommand.
- void addArgument() {
- if (!FullyInitialized)
- addArgument(*TopLevelSubCommand);
- }
+ void addArgument();
/// Unregisters this option from the CommandLine system.
///
@@ -469,7 +465,7 @@ struct sub {
sub(SubCommand &S) : Sub(S) {}
- template <class Opt> void apply(Opt &O) const { O.addArgument(Sub); }
+ template <class Opt> void apply(Opt &O) const { O.addSubCommand(Sub); }
};
//===----------------------------------------------------------------------===//
@@ -1776,10 +1772,11 @@ class alias : public Option {
error("cl::alias must have argument name specified!");
if (!AliasFor)
error("cl::alias must have an cl::aliasopt(option) specified!");
- for(OptionCategory *Cat: AliasFor->getCategories())
- addCategory(*Cat);
- for(SubCommand *SC: AliasFor->getSubCommands())
- Option::addArgument(*SC);
+ if (!Subs.empty())
+ error("cl::alias must not have cl::sub(), aliased option's cl::sub() will be used!");
+ Subs = AliasFor->Subs;
+ Categories = AliasFor->Categories;
+ addArgument();
}
public:
@@ -1793,10 +1790,6 @@ public:
AliasFor = &O;
}
- // Does nothing when called via apply. Aliases call Option::addArgument
- // directly in the done() method to actually add the option..
- void addArgument(SubCommand &SC) override {}
-
template <class... Mods>
explicit alias(const Mods &... Ms)
: Option(Optional, Hidden), AliasFor(nullptr) {
Modified: llvm/trunk/lib/Support/CommandLine.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=365742&r1=365741&r2=365742&view=diff
==============================================================================
--- llvm/trunk/lib/Support/CommandLine.cpp (original)
+++ llvm/trunk/lib/Support/CommandLine.cpp Thu Jul 11 01:54:28 2019
@@ -142,7 +142,7 @@ public:
// This collects Options added with the cl::DefaultOption flag. Since they can
// be overridden, they are not added to the appropriate SubCommands until
// ParseCommandLineOptions actually runs.
- SmallVector<std::pair<Option*, SubCommand*>, 4> DefaultOptions;
+ SmallVector<Option*, 4> DefaultOptions;
// This collects the different option categories that have been registered.
SmallPtrSet<OptionCategory *, 16> RegisteredOptionCategories;
@@ -151,7 +151,6 @@ public:
SmallPtrSet<SubCommand *, 4> RegisteredSubCommands;
CommandLineParser() : ActiveSubCommand(nullptr) {
- RegisteredOptionCategories.insert(&*GeneralCategory);
registerSubCommand(&*TopLevelSubCommand);
registerSubCommand(&*AllSubCommands);
}
@@ -183,16 +182,15 @@ public:
}
void addLiteralOption(Option &Opt, StringRef Name) {
- for(SubCommand *SC: Opt.getSubCommands())
- addLiteralOption(Opt, SC, Name);
- }
-
- void addOption(Option *O, SubCommand *SC, bool ProcessDefaultOptions = false) {
- if (!ProcessDefaultOptions && O->isDefaultOption()) {
- DefaultOptions.push_back(std::make_pair(O, SC));
- return;
+ if (Opt.Subs.empty())
+ addLiteralOption(Opt, &*TopLevelSubCommand, Name);
+ else {
+ for (auto SC : Opt.Subs)
+ addLiteralOption(Opt, SC, Name);
}
+ }
+ void addOption(Option *O, SubCommand *SC) {
bool HadErrors = false;
if (O->hasArgStr()) {
// If it's a DefaultOption, check to make sure it isn't already there.
@@ -234,14 +232,22 @@ public:
for (const auto &Sub : RegisteredSubCommands) {
if (SC == Sub)
continue;
- addOption(O, Sub, ProcessDefaultOptions);
+ addOption(O, Sub);
}
}
}
- void addDefaultOptions() {
- for (std::pair<Option *, SubCommand *> &DO : DefaultOptions) {
- addOption(DO.first, DO.second, true);
+ void addOption(Option *O, bool ProcessDefaultOption = false) {
+ if (!ProcessDefaultOption && O->isDefaultOption()) {
+ DefaultOptions.push_back(O);
+ return;
+ }
+
+ if (O->Subs.empty()) {
+ addOption(O, &*TopLevelSubCommand);
+ } else {
+ for (auto SC : O->Subs)
+ addOption(O, SC);
}
}
@@ -279,8 +285,17 @@ public:
}
void removeOption(Option *O) {
- for (auto SC : RegisteredSubCommands)
- removeOption(O, SC);
+ if (O->Subs.empty())
+ removeOption(O, &*TopLevelSubCommand);
+ else {
+ if (O->isInAllSubCommands()) {
+ for (auto SC : RegisteredSubCommands)
+ removeOption(O, SC);
+ } else {
+ for (auto SC : O->Subs)
+ removeOption(O, SC);
+ }
+ }
}
bool hasOptions(const SubCommand &Sub) const {
@@ -309,8 +324,17 @@ public:
}
void updateArgStr(Option *O, StringRef NewName) {
- for (auto SC : RegisteredSubCommands)
- updateArgStr(O, NewName, SC);
+ if (O->Subs.empty())
+ updateArgStr(O, NewName, &*TopLevelSubCommand);
+ else {
+ if (O->isInAllSubCommands()) {
+ for (auto SC : RegisteredSubCommands)
+ updateArgStr(O, NewName, SC);
+ } else {
+ for (auto SC : O->Subs)
+ updateArgStr(O, NewName, SC);
+ }
+ }
}
void printOptionValues();
@@ -365,7 +389,6 @@ public:
MoreHelp.clear();
RegisteredOptionCategories.clear();
- RegisteredOptionCategories.insert(&*GeneralCategory);
ResetAllOptionOccurrences();
RegisteredSubCommands.clear();
@@ -404,38 +427,13 @@ extrahelp::extrahelp(StringRef Help) : m
GlobalParser->MoreHelp.push_back(Help);
}
-void Option::addArgument(SubCommand &SC) {
- GlobalParser->addOption(this, &SC);
+void Option::addArgument() {
+ GlobalParser->addOption(this);
FullyInitialized = true;
}
void Option::removeArgument() { GlobalParser->removeOption(this); }
-SmallPtrSet<OptionCategory *, 1> Option::getCategories() const {
- SmallPtrSet<OptionCategory *, 1> Cats;
- for (OptionCategory *C: GlobalParser->RegisteredOptionCategories) {
- if (C->MemberOptions.find(this) != C->MemberOptions.end())
- Cats.insert(C);
- }
- if (Cats.empty())
- Cats.insert(&*GeneralCategory);
- return Cats;
-}
-
-SmallPtrSet<SubCommand *, 1> Option::getSubCommands() const {
- // This can happen for enums and literal options.
- if (ArgStr.empty())
- return SmallPtrSet<SubCommand *, 1>{&*TopLevelSubCommand};
-
- SmallPtrSet<SubCommand *, 1> Subs;
- for (SubCommand *SC : GlobalParser->getRegisteredSubcommands()) {
- auto I = SC->OptionsMap.find(ArgStr);
- if (I != SC->OptionsMap.end() && I->getValue() == this)
- Subs.insert(SC);
- }
- return Subs;
-}
-
void Option::setArgStr(StringRef S) {
if (FullyInitialized)
GlobalParser->updateArgStr(this, S);
@@ -446,7 +444,14 @@ void Option::setArgStr(StringRef S) {
}
void Option::addCategory(OptionCategory &C) {
- C.MemberOptions.insert(this);
+ assert(!Categories.empty() && "Categories cannot be empty.");
+ // Maintain backward compatibility by replacing the default GeneralCategory
+ // if it's still set. Otherwise, just add the new one. The GeneralCategory
+ // must be explicitly added if you want multiple categories that include it.
+ if (&C != &GeneralCategory && Categories[0] == &GeneralCategory)
+ Categories[0] = &C;
+ else if (find(Categories, &C) == Categories.end())
+ Categories.push_back(&C);
}
void Option::reset() {
@@ -457,8 +462,7 @@ void Option::reset() {
}
// Initialise the general option category.
-LLVM_REQUIRE_CONSTANT_INITIALIZATION
-ManagedStatic<OptionCategory> llvm::cl::GeneralCategory;
+OptionCategory llvm::cl::GeneralCategory("General options");
void OptionCategory::registerCategory() {
GlobalParser->registerCategory(this);
@@ -1298,7 +1302,9 @@ bool CommandLineParser::ParseCommandLine
auto &SinkOpts = ChosenSubCommand->SinkOpts;
auto &OptionsMap = ChosenSubCommand->OptionsMap;
- addDefaultOptions();
+ for (auto O: DefaultOptions) {
+ addOption(O, true);
+ }
if (ConsumeAfterOpt) {
assert(PositionalOpts.size() > 0 &&
@@ -2198,7 +2204,7 @@ protected:
// options within categories will also be alphabetically sorted.
for (size_t I = 0, E = Opts.size(); I != E; ++I) {
Option *Opt = Opts[I].second;
- for (auto *Cat : Opt->getCategories()) {
+ for (auto &Cat : Opt->Categories) {
assert(CategorizedOptions.count(Cat) > 0 &&
"Option has an unregistered category");
CategorizedOptions[Cat].push_back(Opt);
@@ -2459,7 +2465,7 @@ cl::getRegisteredSubcommands() {
void cl::HideUnrelatedOptions(cl::OptionCategory &Category, SubCommand &Sub) {
for (auto &I : Sub.OptionsMap) {
- for (OptionCategory *Cat : I.second->getCategories()) {
+ for (auto &Cat : I.second->Categories) {
if (Cat != &Category &&
Cat != &GenericCategory)
I.second->setHiddenFlag(cl::ReallyHidden);
@@ -2470,7 +2476,7 @@ void cl::HideUnrelatedOptions(cl::Option
void cl::HideUnrelatedOptions(ArrayRef<const cl::OptionCategory *> Categories,
SubCommand &Sub) {
for (auto &I : Sub.OptionsMap) {
- for (OptionCategory *Cat : I.second->getCategories()) {
+ for (auto &Cat : I.second->Categories) {
if (find(Categories, Cat) == Categories.end() && Cat != &GenericCategory)
I.second->setHiddenFlag(cl::ReallyHidden);
}
Modified: llvm/trunk/unittests/Support/CommandLineTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CommandLineTest.cpp?rev=365742&r1=365741&r2=365742&view=diff
==============================================================================
--- llvm/trunk/unittests/Support/CommandLineTest.cpp (original)
+++ llvm/trunk/unittests/Support/CommandLineTest.cpp Thu Jul 11 01:54:28 2019
@@ -95,16 +95,16 @@ TEST(CommandLineTest, ModifyExisitingOpt
cl::Option *Retrieved = Map["test-option"];
ASSERT_EQ(&TestOption, Retrieved) << "Retrieved wrong option.";
- ASSERT_NE(Retrieved->getCategories().end(),
- find_if(Retrieved->getCategories(),
+ ASSERT_NE(Retrieved->Categories.end(),
+ find_if(Retrieved->Categories,
[&](const llvm::cl::OptionCategory *Cat) {
- return Cat == &*cl::GeneralCategory;
+ return Cat == &cl::GeneralCategory;
}))
<< "Incorrect default option category.";
Retrieved->addCategory(TestCategory);
- ASSERT_NE(Retrieved->getCategories().end(),
- find_if(Retrieved->getCategories(),
+ ASSERT_NE(Retrieved->Categories.end(),
+ find_if(Retrieved->Categories,
[&](const llvm::cl::OptionCategory *Cat) {
return Cat == &TestCategory;
}))
@@ -160,8 +160,8 @@ TEST(CommandLineTest, ParseEnvironmentTo
TEST(CommandLineTest, UseOptionCategory) {
StackOption<int> TestOption2("test-option", cl::cat(TestCategory));
- ASSERT_NE(TestOption2.getCategories().end(),
- find_if(TestOption2.getCategories(),
+ ASSERT_NE(TestOption2.Categories.end(),
+ find_if(TestOption2.Categories,
[&](const llvm::cl::OptionCategory *Cat) {
return Cat == &TestCategory;
}))
@@ -170,44 +170,42 @@ TEST(CommandLineTest, UseOptionCategory)
TEST(CommandLineTest, UseMultipleCategories) {
StackOption<int> TestOption2("test-option2", cl::cat(TestCategory),
- cl::cat(*cl::GeneralCategory),
- cl::cat(*cl::GeneralCategory));
- auto TestOption2Categories = TestOption2.getCategories();
+ cl::cat(cl::GeneralCategory),
+ cl::cat(cl::GeneralCategory));
// Make sure cl::GeneralCategory wasn't added twice.
- ASSERT_EQ(TestOption2Categories.size(), 2U);
+ ASSERT_EQ(TestOption2.Categories.size(), 2U);
- ASSERT_NE(TestOption2Categories.end(),
- find_if(TestOption2Categories,
+ ASSERT_NE(TestOption2.Categories.end(),
+ find_if(TestOption2.Categories,
[&](const llvm::cl::OptionCategory *Cat) {
return Cat == &TestCategory;
}))
<< "Failed to assign Option Category.";
- ASSERT_NE(TestOption2Categories.end(),
- find_if(TestOption2Categories,
+ ASSERT_NE(TestOption2.Categories.end(),
+ find_if(TestOption2.Categories,
[&](const llvm::cl::OptionCategory *Cat) {
- return Cat == &*cl::GeneralCategory;
+ return Cat == &cl::GeneralCategory;
}))
<< "Failed to assign General Category.";
cl::OptionCategory AnotherCategory("Additional test Options", "Description");
StackOption<int> TestOption("test-option", cl::cat(TestCategory),
cl::cat(AnotherCategory));
- auto TestOptionCategories = TestOption.getCategories();
- ASSERT_EQ(TestOptionCategories.end(),
- find_if(TestOptionCategories,
+ ASSERT_EQ(TestOption.Categories.end(),
+ find_if(TestOption.Categories,
[&](const llvm::cl::OptionCategory *Cat) {
- return Cat == &*cl::GeneralCategory;
+ return Cat == &cl::GeneralCategory;
}))
<< "Failed to remove General Category.";
- ASSERT_NE(TestOptionCategories.end(),
- find_if(TestOptionCategories,
+ ASSERT_NE(TestOption.Categories.end(),
+ find_if(TestOption.Categories,
[&](const llvm::cl::OptionCategory *Cat) {
return Cat == &TestCategory;
}))
<< "Failed to assign Option Category.";
- ASSERT_NE(TestOptionCategories.end(),
- find_if(TestOptionCategories,
+ ASSERT_NE(TestOption.Categories.end(),
+ find_if(TestOption.Categories,
[&](const llvm::cl::OptionCategory *Cat) {
return Cat == &AnotherCategory;
}))
@@ -380,30 +378,6 @@ TEST(CommandLineTest, AliasRequired) {
testAliasRequired(array_lengthof(opts2), opts2);
}
-TEST(CommandLineTest, AliasWithSubCommand) {
- StackSubCommand SC1("sc1", "Subcommand 1");
- StackOption<std::string> Option1("option", cl::value_desc("output file"),
- cl::init("-"), cl::desc("Option"),
- cl::sub(SC1));
- StackOption<std::string, cl::alias> Alias1("o", llvm::cl::aliasopt(Option1),
- cl::desc("Alias for --option"),
- cl::sub(SC1));
-}
-
-TEST(CommandLineTest, AliasWithMultipleSubCommandsWithSameOption) {
- StackSubCommand SC1("sc1", "Subcommand 1");
- StackOption<std::string> Option1("option", cl::value_desc("output file"),
- cl::init("-"), cl::desc("Option"),
- cl::sub(SC1));
- StackSubCommand SC2("sc2", "Subcommand 2");
- StackOption<std::string> Option2("option", cl::value_desc("output file"),
- cl::init("-"), cl::desc("Option"),
- cl::sub(SC2));
-
- StackOption<std::string, cl::alias> Alias1("o", llvm::cl::aliasopt(Option1),
- cl::desc("Alias for --option"));
-}
-
TEST(CommandLineTest, HideUnrelatedOptions) {
StackOption<int> TestOption1("hide-option-1");
StackOption<int> TestOption2("hide-option-2", cl::cat(TestCategory));
More information about the llvm-commits
mailing list