[CommandLine][NFCI] Simplify enumerating subcommands of an option

[igorkudrin]

The patch adds a helper method to iterate over all subcommands to which an option belongs. Duplicate code is removed and replaced with calls to this new method.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Igor Kudrin (igorkudrin)

Changes

The patch adds a helper method to iterate over all subcommands to which an option belongs. Duplicate code is removed and replaced with calls to this new method.

---

Full diff: https://github.com/llvm/llvm-project/pull/75679.diff

2 Files Affected:

• (modified) llvm/include/llvm/Support/CommandLine.h (-4)
• (modified) llvm/lib/Support/CommandLine.cpp (+24-55)

diff --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h
index 58ef176551b685..5d733eeee6d399 100644
--- a/llvm/include/llvm/Support/CommandLine.h
+++ b/llvm/include/llvm/Support/CommandLine.h
@@ -314,10 +314,6 @@ class Option {
     return getNumOccurrencesFlag() == cl::ConsumeAfter;
   }
 
-  bool isInAllSubCommands() const {
-    return Subs.contains(&SubCommand::getAll());
-  }
-
   //-------------------------------------------------------------------------===
   // Accessor functions set by OptionModifiers
   //
diff --git a/llvm/lib/Support/CommandLine.cpp b/llvm/lib/Support/CommandLine.cpp
index 088b4e4d755cbe..00179bc32551fe 100644
--- a/llvm/lib/Support/CommandLine.cpp
+++ b/llvm/lib/Support/CommandLine.cpp
@@ -175,6 +175,24 @@ class CommandLineParser {
                                StringRef Overview, raw_ostream *Errs = nullptr,
                                bool LongOptionsUseDoubleDash = false);
 
+  void forEachSubCommand(Option &Opt,
+                         std::function<void(SubCommand &)> Action) {
+    if (Opt.Subs.empty()) {
+      Action(SubCommand::getTopLevel());
+      return;
+    }
+    if (Opt.Subs.size() == 1 && *Opt.Subs.begin() == &SubCommand::getAll()) {
+      for (auto *SC : RegisteredSubCommands)
+        Action(*SC);
+      return;
+    }
+    for (auto *SC : Opt.Subs) {
+      assert(SC != &SubCommand::getAll() &&
+             "SubCommand::getAll() should not be used with other subcommands");
+      Action(*SC);
+    }
+  }
+
   void addLiteralOption(Option &Opt, SubCommand *SC, StringRef Name) {
     if (Opt.hasArgStr())
       return;
@@ -183,25 +201,11 @@ class CommandLineParser {
              << "' registered more than once!\n";
       report_fatal_error("inconsistency in registered CommandLine options");
     }
-
-    // If we're adding this to all sub-commands, add it to the ones that have
-    // already been registered.
-    if (SC == &SubCommand::getAll()) {
-      for (auto *Sub : RegisteredSubCommands) {
-        if (SC == Sub)
-          continue;
-        addLiteralOption(Opt, Sub, Name);
-      }
-    }
   }
 
   void addLiteralOption(Option &Opt, StringRef Name) {
-    if (Opt.Subs.empty())
-      addLiteralOption(Opt, &SubCommand::getTopLevel(), Name);
-    else {
-      for (auto *SC : Opt.Subs)
-        addLiteralOption(Opt, SC, Name);
-    }
+    forEachSubCommand(
+        Opt, [&](SubCommand &SC) { addLiteralOption(Opt, &SC, Name); });
   }
 
   void addOption(Option *O, SubCommand *SC) {
@@ -238,16 +242,6 @@ class CommandLineParser {
     // linked LLVM distribution.
     if (HadErrors)
       report_fatal_error("inconsistency in registered CommandLine options");
-
-    // If we're adding this to all sub-commands, add it to the ones that have
-    // already been registered.
-    if (SC == &SubCommand::getAll()) {
-      for (auto *Sub : RegisteredSubCommands) {
-        if (SC == Sub)
-          continue;
-        addOption(O, Sub);
-      }
-    }
   }
 
   void addOption(Option *O, bool ProcessDefaultOption = false) {
@@ -255,13 +249,7 @@ class CommandLineParser {
       DefaultOptions.push_back(O);
       return;
     }
-
-    if (O->Subs.empty()) {
-      addOption(O, &SubCommand::getTopLevel());
-    } else {
-      for (auto *SC : O->Subs)
-        addOption(O, SC);
-    }
+    forEachSubCommand(*O, [&](SubCommand &SC) { addOption(O, &SC); });
   }
 
   void removeOption(Option *O, SubCommand *SC) {
@@ -298,17 +286,7 @@ class CommandLineParser {
   }
 
   void removeOption(Option *O) {
-    if (O->Subs.empty())
-      removeOption(O, &SubCommand::getTopLevel());
-    else {
-      if (O->isInAllSubCommands()) {
-        for (auto *SC : RegisteredSubCommands)
-          removeOption(O, SC);
-      } else {
-        for (auto *SC : O->Subs)
-          removeOption(O, SC);
-      }
-    }
+    forEachSubCommand(*O, [&](SubCommand &SC) { removeOption(O, &SC); });
   }
 
   bool hasOptions(const SubCommand &Sub) const {
@@ -344,17 +322,8 @@ class CommandLineParser {
   }
 
   void updateArgStr(Option *O, StringRef NewName) {
-    if (O->Subs.empty())
-      updateArgStr(O, NewName, &SubCommand::getTopLevel());
-    else {
-      if (O->isInAllSubCommands()) {
-        for (auto *SC : RegisteredSubCommands)
-          updateArgStr(O, NewName, SC);
-      } else {
-        for (auto *SC : O->Subs)
-          updateArgStr(O, NewName, SC);
-      }
-    }
+    forEachSubCommand(*O,
+                      [&](SubCommand &SC) { updateArgStr(O, NewName, &SC); });
   }
 
   void printOptionValues();

[jh7370]

LGTM.

[RKSimon]

LGTM

[nikic]

Please use function_ref instead of std::function here.

[igorkudrin]

Thanks for the suggestion. I've created #75973 for this.