From: Don Hinton <hintonda@gmail.com>
Date: Wed, 10 Jul 2019 17:57:05 +0000 (+0000)
Subject: Recommit "[CommandLine] Remove OptionCategory and SubCommand caches from the Option... 
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f9b7c138eccf70cfc06901343464c62f27568930;p=llvm

Recommit "[CommandLine] Remove OptionCategory and SubCommand caches from the Option class."

Previously reverted in 364141 due to buildbot breakage, and fixed here
by making GeneralCategory global a ManagedStatic.

Summary:
This change processes `OptionCategory`s and `SubCommand`s as they
are seen instead of caching them in the Option class and processing
them later.  Doing so simplifies the work needed to be done by the Global
parser and significantly reduces the size of the Option class to a mere 64
bytes.

Removing  the `OptionCategory` cache saved 24 bytes, and removing
the `SubCommand` cache saved an additional 48 bytes, for a total of a
72 byte reduction.

Reviewed By: serge-sans-paille

Tags: #llvm, #clang

Differential Revision: https://reviews.llvm.org/D62105

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@365675 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/include/llvm/Support/CommandLine.h b/include/llvm/Support/CommandLine.h
index 3cc2c3c0121..036830282a5 100644
--- a/include/llvm/Support/CommandLine.h
+++ b/include/llvm/Support/CommandLine.h
@@ -187,24 +187,27 @@ enum MiscFlags {             // Miscellaneous flags to adjust argument
 //
 class OptionCategory {
 private:
-  StringRef const Name;
-  StringRef const Description;
+  StringRef Name = "General Category";
+  StringRef Description;
 
   void registerCategory();
 
 public:
-  OptionCategory(StringRef const Name,
-                 StringRef const Description = "")
+  OptionCategory(StringRef Name,
+                 StringRef 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 OptionCategory GeneralCategory;
+extern ManagedStatic<OptionCategory> GeneralCategory;
 
 //===----------------------------------------------------------------------===//
 // SubCommand class
@@ -283,9 +286,12 @@ 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
-  SmallVector<OptionCategory *, 1>
-      Categories;                    // The Categories this option belongs to
-  SmallPtrSet<SubCommand *, 1> Subs; // The subcommands this option belongs to.
+
+  // 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;
 
   inline enum NumOccurrencesFlag getNumOccurrencesFlag() const {
     return (enum NumOccurrencesFlag)Occurrences;
@@ -317,12 +323,6 @@ public:
     return getNumOccurrencesFlag() == cl::ConsumeAfter;
   }
 
-  bool isInAllSubCommands() const {
-    return any_of(Subs, [](const SubCommand *SC) {
-      return SC == &*AllSubCommands;
-    });
-  }
-
   //-------------------------------------------------------------------------===
   // Accessor functions set by OptionModifiers
   //
@@ -336,16 +336,13 @@ 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) {
-    Categories.push_back(&GeneralCategory);
-  }
+        FullyInitialized(false), Position(0), AdditionalVals(0) {}
 
   inline void setNumAdditionalVals(unsigned n) { AdditionalVals = n; }
 
@@ -354,7 +351,14 @@ public:
 
   // addArgument - Register this argument with the commandline system.
   //
-  void addArgument();
+  virtual void addArgument(SubCommand &SC);
+
+  // addArgument - Only called in done() method to add default
+  // TopLevelSubCommand.
+  void addArgument() {
+    if (!FullyInitialized)
+      addArgument(*TopLevelSubCommand);
+  }
 
   /// Unregisters this option from the CommandLine system.
   ///
@@ -465,7 +469,7 @@ struct sub {
 
   sub(SubCommand &S) : Sub(S) {}
 
-  template <class Opt> void apply(Opt &O) const { O.addSubCommand(Sub); }
+  template <class Opt> void apply(Opt &O) const { O.addArgument(Sub); }
 };
 
 //===----------------------------------------------------------------------===//
@@ -1772,11 +1776,10 @@ class alias : public Option {
       error("cl::alias must have argument name specified!");
     if (!AliasFor)
       error("cl::alias must have an cl::aliasopt(option) specified!");
-    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();
+    for(OptionCategory *Cat: AliasFor->getCategories())
+      addCategory(*Cat);
+    for(SubCommand *SC: AliasFor->getSubCommands())
+      Option::addArgument(*SC);
   }
 
 public:
@@ -1790,6 +1793,10 @@ 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) {
diff --git a/lib/Support/CommandLine.cpp b/lib/Support/CommandLine.cpp
index 25510fa58ff..036c2d1092c 100644
--- a/lib/Support/CommandLine.cpp
+++ b/lib/Support/CommandLine.cpp
@@ -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<Option*, 4> DefaultOptions;
+  SmallVector<std::pair<Option*, SubCommand*>, 4> DefaultOptions;
 
   // This collects the different option categories that have been registered.
   SmallPtrSet<OptionCategory *, 16> RegisteredOptionCategories;
@@ -151,6 +151,7 @@ public:
   SmallPtrSet<SubCommand *, 4> RegisteredSubCommands;
 
   CommandLineParser() : ActiveSubCommand(nullptr) {
+    RegisteredOptionCategories.insert(&*GeneralCategory);
     registerSubCommand(&*TopLevelSubCommand);
     registerSubCommand(&*AllSubCommands);
   }
@@ -182,15 +183,16 @@ public:
   }
 
   void addLiteralOption(Option &Opt, StringRef Name) {
-    if (Opt.Subs.empty())
-      addLiteralOption(Opt, &*TopLevelSubCommand, Name);
-    else {
-      for (auto SC : Opt.Subs)
-        addLiteralOption(Opt, SC, Name);
-    }
+    for(SubCommand *SC: Opt.getSubCommands())
+      addLiteralOption(Opt, SC, Name);
   }
 
-  void addOption(Option *O, SubCommand *SC) {
+  void addOption(Option *O, SubCommand *SC, bool ProcessDefaultOptions = false) {
+    if (!ProcessDefaultOptions && O->isDefaultOption()) {
+      DefaultOptions.push_back(std::make_pair(O, SC));
+      return;
+    }
+
     bool HadErrors = false;
     if (O->hasArgStr()) {
       // If it's a DefaultOption, check to make sure it isn't already there.
@@ -232,22 +234,14 @@ public:
       for (const auto &Sub : RegisteredSubCommands) {
         if (SC == Sub)
           continue;
-        addOption(O, Sub);
+        addOption(O, Sub, ProcessDefaultOptions);
       }
     }
   }
 
-  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);
+  void addDefaultOptions() {
+    for (std::pair<Option *, SubCommand *> &DO : DefaultOptions) {
+      addOption(DO.first, DO.second, true);
     }
   }
 
@@ -285,17 +279,8 @@ public:
   }
 
   void removeOption(Option *O) {
-    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);
-      }
-    }
+    for (auto SC : RegisteredSubCommands)
+      removeOption(O, SC);
   }
 
   bool hasOptions(const SubCommand &Sub) const {
@@ -324,17 +309,8 @@ public:
   }
 
   void updateArgStr(Option *O, StringRef NewName) {
-    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);
-      }
-    }
+    for (auto SC : RegisteredSubCommands)
+      updateArgStr(O, NewName, SC);
   }
 
   void printOptionValues();
@@ -389,6 +365,7 @@ public:
 
     MoreHelp.clear();
     RegisteredOptionCategories.clear();
+    RegisteredOptionCategories.insert(&*GeneralCategory);
 
     ResetAllOptionOccurrences();
     RegisteredSubCommands.clear();
@@ -427,13 +404,38 @@ extrahelp::extrahelp(StringRef Help) : morehelp(Help) {
   GlobalParser->MoreHelp.push_back(Help);
 }
 
-void Option::addArgument() {
-  GlobalParser->addOption(this);
+void Option::addArgument(SubCommand &SC) {
+  GlobalParser->addOption(this, &SC);
   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);
@@ -444,14 +446,7 @@ void Option::setArgStr(StringRef S) {
 }
 
 void Option::addCategory(OptionCategory &C) {
-  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);
+  C.MemberOptions.insert(this);
 }
 
 void Option::reset() {
@@ -462,7 +457,8 @@ void Option::reset() {
 }
 
 // Initialise the general option category.
-OptionCategory llvm::cl::GeneralCategory("General options");
+LLVM_REQUIRE_CONSTANT_INITIALIZATION
+ManagedStatic<OptionCategory> llvm::cl::GeneralCategory;
 
 void OptionCategory::registerCategory() {
   GlobalParser->registerCategory(this);
@@ -1302,9 +1298,7 @@ bool CommandLineParser::ParseCommandLineOptions(int argc,
   auto &SinkOpts = ChosenSubCommand->SinkOpts;
   auto &OptionsMap = ChosenSubCommand->OptionsMap;
 
-  for (auto O: DefaultOptions) {
-    addOption(O, true);
-  }
+  addDefaultOptions();
 
   if (ConsumeAfterOpt) {
     assert(PositionalOpts.size() > 0 &&
@@ -2204,7 +2198,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->Categories) {
+      for (auto *Cat : Opt->getCategories()) {
         assert(CategorizedOptions.count(Cat) > 0 &&
                "Option has an unregistered category");
         CategorizedOptions[Cat].push_back(Opt);
@@ -2465,7 +2459,7 @@ cl::getRegisteredSubcommands() {
 
 void cl::HideUnrelatedOptions(cl::OptionCategory &Category, SubCommand &Sub) {
   for (auto &I : Sub.OptionsMap) {
-    for (auto &Cat : I.second->Categories) {
+    for (OptionCategory *Cat : I.second->getCategories()) {
       if (Cat != &Category &&
           Cat != &GenericCategory)
         I.second->setHiddenFlag(cl::ReallyHidden);
@@ -2476,7 +2470,7 @@ void cl::HideUnrelatedOptions(cl::OptionCategory &Category, SubCommand &Sub) {
 void cl::HideUnrelatedOptions(ArrayRef<const cl::OptionCategory *> Categories,
                               SubCommand &Sub) {
   for (auto &I : Sub.OptionsMap) {
-    for (auto &Cat : I.second->Categories) {
+    for (OptionCategory *Cat : I.second->getCategories()) {
       if (find(Categories, Cat) == Categories.end() && Cat != &GenericCategory)
         I.second->setHiddenFlag(cl::ReallyHidden);
     }
diff --git a/unittests/Support/CommandLineTest.cpp b/unittests/Support/CommandLineTest.cpp
index 1a1c8a4404e..f180e053493 100644
--- a/unittests/Support/CommandLineTest.cpp
+++ b/unittests/Support/CommandLineTest.cpp
@@ -95,16 +95,16 @@ TEST(CommandLineTest, ModifyExisitingOption) {
   cl::Option *Retrieved = Map["test-option"];
   ASSERT_EQ(&TestOption, Retrieved) << "Retrieved wrong option.";
 
-  ASSERT_NE(Retrieved->Categories.end(),
-            find_if(Retrieved->Categories,
+  ASSERT_NE(Retrieved->getCategories().end(),
+            find_if(Retrieved->getCategories(),
                     [&](const llvm::cl::OptionCategory *Cat) {
-                      return Cat == &cl::GeneralCategory;
+                      return Cat == &*cl::GeneralCategory;
                     }))
       << "Incorrect default option category.";
 
   Retrieved->addCategory(TestCategory);
-  ASSERT_NE(Retrieved->Categories.end(),
-            find_if(Retrieved->Categories,
+  ASSERT_NE(Retrieved->getCategories().end(),
+            find_if(Retrieved->getCategories(),
                     [&](const llvm::cl::OptionCategory *Cat) {
                       return Cat == &TestCategory;
                     }))
@@ -160,8 +160,8 @@ TEST(CommandLineTest, ParseEnvironmentToLocalVar) {
 TEST(CommandLineTest, UseOptionCategory) {
   StackOption<int> TestOption2("test-option", cl::cat(TestCategory));
 
-  ASSERT_NE(TestOption2.Categories.end(),
-            find_if(TestOption2.Categories,
+  ASSERT_NE(TestOption2.getCategories().end(),
+            find_if(TestOption2.getCategories(),
                          [&](const llvm::cl::OptionCategory *Cat) {
                            return Cat == &TestCategory;
                          }))
@@ -170,42 +170,44 @@ TEST(CommandLineTest, UseOptionCategory) {
 
 TEST(CommandLineTest, UseMultipleCategories) {
   StackOption<int> TestOption2("test-option2", cl::cat(TestCategory),
-                               cl::cat(cl::GeneralCategory),
-                               cl::cat(cl::GeneralCategory));
+                               cl::cat(*cl::GeneralCategory),
+                               cl::cat(*cl::GeneralCategory));
+  auto TestOption2Categories = TestOption2.getCategories();
 
   // Make sure cl::GeneralCategory wasn't added twice.
-  ASSERT_EQ(TestOption2.Categories.size(), 2U);
+  ASSERT_EQ(TestOption2Categories.size(), 2U);
 
-  ASSERT_NE(TestOption2.Categories.end(),
-            find_if(TestOption2.Categories,
+  ASSERT_NE(TestOption2Categories.end(),
+            find_if(TestOption2Categories,
                          [&](const llvm::cl::OptionCategory *Cat) {
                            return Cat == &TestCategory;
                          }))
       << "Failed to assign Option Category.";
-  ASSERT_NE(TestOption2.Categories.end(),
-            find_if(TestOption2.Categories,
+  ASSERT_NE(TestOption2Categories.end(),
+            find_if(TestOption2Categories,
                          [&](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));
-  ASSERT_EQ(TestOption.Categories.end(),
-            find_if(TestOption.Categories,
+  auto TestOptionCategories = TestOption.getCategories();
+  ASSERT_EQ(TestOptionCategories.end(),
+            find_if(TestOptionCategories,
                          [&](const llvm::cl::OptionCategory *Cat) {
-                           return Cat == &cl::GeneralCategory;
+                           return Cat == &*cl::GeneralCategory;
                          }))
       << "Failed to remove General Category.";
-  ASSERT_NE(TestOption.Categories.end(),
-            find_if(TestOption.Categories,
+  ASSERT_NE(TestOptionCategories.end(),
+            find_if(TestOptionCategories,
                          [&](const llvm::cl::OptionCategory *Cat) {
                            return Cat == &TestCategory;
                          }))
       << "Failed to assign Option Category.";
-  ASSERT_NE(TestOption.Categories.end(),
-            find_if(TestOption.Categories,
+  ASSERT_NE(TestOptionCategories.end(),
+            find_if(TestOptionCategories,
                          [&](const llvm::cl::OptionCategory *Cat) {
                            return Cat == &AnotherCategory;
                          }))
@@ -378,6 +380,30 @@ 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));