[CommandLine] Move categories out of Option and into parser [NFC]

[hintonda]

Summary

This change reduces the size of each Option by only storing category
info that's actually used in the global parser. Saves 24 bytes per
Option.

[MaskRay]

I recall that you have a patch that touched this area, which was committed and reverted for several times. Is this patch a similar one? Can you find pointers to the old one?

Have you tried TinyPtrVector?

[hintonda]

In D71438#1787248, @MaskRay wrote:

I recall that you have a patch that touched this area, which was committed and reverted for several times. Is this patch a similar one? Can you find pointers to the old one?

Yes, the previous patch, D62105, attempted to move both SubCommands and Categories at the same time, but was overly complicated. I finally abandoned it after I was unable to reproduce a buildbot failure. This patch is similar, but only moves Categories and is much simpler.

Have you tried TinyPtrVector?

No, but I'll take a look tonight -- probably a better option than SmallPtrSet, especially since it's per Option, and we don't need to worry about dups in that context.

llvm/include/llvm/Support/CommandLine.h

 //===----------------------------------------------------------------------===//
 // Option Category class
 //
 class OptionCategory {
 private:
   StringRef const Name;
   StringRef const Description;
 
-  void registerCategory();
-
 public:
   OptionCategory(StringRef const Name,
                  StringRef const Description = "")
-      : Name(Name), Description(Description) {
-    registerCategory();
-  }
+      : Name(Name), Description(Description) {}
 
   StringRef getName() const { return Name; }
   StringRef getDescription() const { return Description; }
 };
 
 // The general Option Category (used as default category).
 extern OptionCategory GeneralCategory;
 
class Option {
   uint16_t FullyInitialized : 1; // Has addArgument been called?
   uint16_t Position;             // Position of last occurrence of the option
   uint16_t AdditionalVals;       // Greater than 0 for multi-valued option.
 
 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();
+
   inline enum NumOccurrencesFlag getNumOccurrencesFlag() const {
     return (enum NumOccurrencesFlag)Occurrences;
   }
 
   inline enum ValueExpected getValueExpectedFlag() const {
     return Value ? ((enum ValueExpected)Value) : getValueExpectedFlagDefault();
   }
 
public:
   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);
   }
 
   inline void setNumAdditionalVals(unsigned n) { AdditionalVals = n; }
 
 public:
   virtual ~Option() = default;
 
   // addArgument - Register this argument with the commandline system.
class alias : public Option {
   void done() {
     if (!hasArgStr())
       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;
+    for(OptionCategory *Cat: AliasFor->getCategories())
+      addCategory(*Cat);
      addArgument();
   }
 
 public:
   // Command line options should not be copyable
   alias(const alias &) = delete;
   alias &operator=(const alias &) = delete;
 
   void setAliasFor(Option &O) {

llvm/lib/Support/CommandLine.cpp

public:
   std::vector<StringRef> MoreHelp;
 
   // 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;
 
   // This collects the different option categories that have been registered.
-  SmallPtrSet<OptionCategory *, 16> RegisteredOptionCategories;
+  std::map<Option *, SmallPtrSet<OptionCategory *, 1>> RegisteredOptionCategories;
 
   // This collects the different subcommands that have been registered.
   SmallPtrSet<SubCommand *, 4> RegisteredSubCommands;
 
   CommandLineParser() : ActiveSubCommand(nullptr) {
     registerSubCommand(&*TopLevelSubCommand);
     registerSubCommand(&*AllSubCommands);
   }
else {
         for (auto SC : O->Subs)
           updateArgStr(O, NewName, SC);
       }
     }
   }
 
   void printOptionValues();
 
-  void registerCategory(OptionCategory *cat) {
-    assert(count_if(RegisteredOptionCategories,
-                    [cat](const OptionCategory *Category) {
-             return cat->getName() == Category->getName();
-           }) == 0 &&
-           "Duplicate option categories");
-
-    RegisteredOptionCategories.insert(cat);
+  void registerCategory(Option *Opt, OptionCategory *cat) {
+    auto &Rec = RegisteredOptionCategories[Opt];
+    Rec.insert(cat);
   }
 
   void registerSubCommand(SubCommand *sub) {
     assert(count_if(RegisteredSubCommands,
                     [sub](const SubCommand *Sub) {
                       return (!sub->getName().empty()) &&
                              (Sub->getName() == sub->getName());
                     }) == 0 &&

 
 void Option::addArgument() {
   GlobalParser->addOption(this);
   FullyInitialized = true;
 }
 
 void Option::removeArgument() { GlobalParser->removeOption(this); }
 
+SmallPtrSet<OptionCategory *, 1> &Option::getCategories() {
+  auto &Cats = GlobalParser->RegisteredOptionCategories[this];
+  if (Cats.empty())
+    GlobalParser->registerCategory(this, &GeneralCategory);
+  return Cats;
+}
+
 void Option::setArgStr(StringRef S) {
   if (FullyInitialized)
     GlobalParser->updateArgStr(this, S);
   assert((S.empty() || S[0] != '-') && "Option can't start with '-");
   ArgStr = S;
   if (ArgStr.size() == 1)
     setMiscFlag(Grouping);
 }
 
 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);
+  GlobalParser->registerCategory(this, &C);
 }
 
 void Option::reset() {
   NumOccurrences = 0;
   setDefault();
   if (isDefaultOption())
     removeArgument();
 }
 
 // Initialise the general option category.
 OptionCategory llvm::cl::GeneralCategory("General options");
 
-void OptionCategory::registerCategory() {
-  GlobalParser->registerCategory(this);
-}
-
 // A special subcommand representing no subcommand. It is particularly important
 // that this ManagedStatic uses constant initailization and not dynamic
 // initialization because it is referenced from cl::opt constructors, which run
 // dynamically in an arbitrary order.
 LLVM_REQUIRE_CONSTANT_INITIALIZATION
 ManagedStatic<SubCommand> llvm::cl::TopLevelSubCommand;
 
 // A special subcommand that can be used to put an option into all subcommands.
void printOptions(StrOptionPairVector &Opts, size_t MaxArgLen) override {
     std::vector<OptionCategory *> SortedCategories;
     std::map<OptionCategory *, std::vector<Option *>> CategorizedOptions;
 
     // Collect registered option categories into vector in preparation for
     // sorting.
     for (auto I = GlobalParser->RegisteredOptionCategories.begin(),
               E = GlobalParser->RegisteredOptionCategories.end();
          I != E; ++I) {
-      SortedCategories.push_back(*I);
+      for (auto *C : I->second) 
+        SortedCategories.push_back(C);
     }
+    SortedCategories.push_back(&GeneralCategory);
 
     // Sort the different option categories alphabetically.
     assert(SortedCategories.size() > 0 && "No option categories registered!");
     array_pod_sort(SortedCategories.begin(), SortedCategories.end(),
                    OptionCategoryCompare);
+    SortedCategories.erase(
+        std::unique(SortedCategories.begin(), SortedCategories.end()),
+        SortedCategories.end());
 
     // Create map to empty vectors.
     for (std::vector<OptionCategory *>::const_iterator
              I = SortedCategories.begin(),
              E = SortedCategories.end();
          I != E; ++I)
       CategorizedOptions[*I] = std::vector<Option *>();
 
     // Walk through pre-sorted options and assign into categories.
     // Because the options are already alphabetically sorted the
     // 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);
       }
     }
 
     // Now do printing.
     for (std::vector<OptionCategory *>::const_iterator

 
 iterator_range<typename SmallPtrSet<SubCommand *, 4>::iterator>
 cl::getRegisteredSubcommands() {
   return GlobalParser->getRegisteredSubcommands();
 }
 
 void cl::HideUnrelatedOptions(cl::OptionCategory &Category, SubCommand &Sub) {
   for (auto &I : Sub.OptionsMap) {
-    for (auto &Cat : I.second->Categories) {
+    for (auto *Cat : I.second->getCategories()) {
       if (Cat != &Category &&
           Cat != &GenericCategory)
         I.second->setHiddenFlag(cl::ReallyHidden);
     }
   }
 }
 
 void cl::HideUnrelatedOptions(ArrayRef<const cl::OptionCategory *> Categories,
                               SubCommand &Sub) {
   for (auto &I : Sub.OptionsMap) {
-    for (auto &Cat : I.second->Categories) {
+    for (auto *Cat : I.second->getCategories()) {
       if (find(Categories, Cat) == Categories.end() && Cat != &GenericCategory)
         I.second->setHiddenFlag(cl::ReallyHidden);
     }
   }
 }
 
 void cl::ResetCommandLineParser() { GlobalParser->reset(); }
 void cl::ResetAllOptionOccurrences() {
   GlobalParser->ResetAllOptionOccurrences();
 }
 
 void LLVMParseCommandLineOptions(int argc, const char *const *argv,
                                  const char *Overview) {
   llvm::cl::ParseCommandLineOptions(argc, argv, StringRef(Overview),
                                     &llvm::nulls());
 }

llvm/unittests/Support/CommandLineTest.cpp

StringMap<cl::Option *> &Map =
       cl::getRegisteredOptions(*cl::TopLevelSubCommand);
 
   ASSERT_TRUE(Map.count("test-option") == 1) <<
     "Could not find option in map.";
 
   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;
                     }))
       << "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;
                     }))
       << "Failed to modify option's option category.";
 
   Retrieved->setDescription(Description);
   ASSERT_STREQ(Retrieved->HelpStr.data(), Description)
       << "Changing option description failed.";
TEST(CommandLineTest, ParseEnvironmentToLocalVar) {
   EXPECT_EQ("hello-local", EnvironmentTestOptionLocal);
 }
 
 #endif  // SKIP_ENVIRONMENT_TESTS
 
 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;
                          }))
       << "Failed to assign Option Category.";
 }
 
 TEST(CommandLineTest, UseMultipleCategories) {
   StackOption<int> TestOption2("test-option2", cl::cat(TestCategory),
                                cl::cat(cl::GeneralCategory),
                                cl::cat(cl::GeneralCategory));
 
   // Make sure cl::GeneralCategory wasn't added twice.
-  ASSERT_EQ(TestOption2.Categories.size(), 2U);
+  ASSERT_EQ(TestOption2.getCategories().size(), 2U);
 
-  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;
                          }))
       << "Failed to assign Option Category.";
-  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 == &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,
+  ASSERT_EQ(TestOption.getCategories().end(),
+            find_if(TestOption.getCategories(),
                          [&](const llvm::cl::OptionCategory *Cat) {
                            return Cat == &cl::GeneralCategory;
                          }))
       << "Failed to remove General Category.";
-  ASSERT_NE(TestOption.Categories.end(),
-            find_if(TestOption.Categories,
+  ASSERT_NE(TestOption.getCategories().end(),
+            find_if(TestOption.getCategories(),
                          [&](const llvm::cl::OptionCategory *Cat) {
                            return Cat == &TestCategory;
                          }))
       << "Failed to assign Option Category.";
-  ASSERT_NE(TestOption.Categories.end(),
-            find_if(TestOption.Categories,
+  ASSERT_NE(TestOption.getCategories().end(),
+            find_if(TestOption.getCategories(),
                          [&](const llvm::cl::OptionCategory *Cat) {
                            return Cat == &AnotherCategory;
                          }))
       << "Failed to assign Another Category.";
 }
 
 typedef void ParserFunction(StringRef Source, StringSaver &Saver,
                             SmallVectorImpl<const char *> &NewArgv,