Clean-up usage of OptionDefinition arrays

[tatyana-krasnukha]

Summary

Removed unused static arrays from headers, removed sentinel entry from OptionDefinition array in Driver.cpp.

[tatyana-krasnukha]

tools/driver/Driver.cpp

71

This type is duplicated here to not break the encapsulation of lldb's private types, am I right?

[clayborg]

tools/driver/Driver.cpp

71

Correct. We want any tool that links against LLDB.framework on darwin, or liblldb.so on other platforms to use the lldb::SB API only.

581–585

Do we even need this if statement and its contents? maybe lldbassert(g_num_options > 0);?

[tatyana-krasnukha]

Modifies Driver::ParseArgs checks

[tatyana-krasnukha]

tools/driver/Driver.cpp

581–585

I replaced it with static_assert since this expression can be checked at compile time. And added if (argc <= 1) to skip build of long_options_vector if there is no arguments to parse.

include/lldb/Target/Platform.h

public:
   lldb_private::Status
   SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
                  ExecutionContext *execution_context) override;
 
   void OptionParsingStarting(ExecutionContext *execution_context) override;
 
   llvm::ArrayRef<OptionDefinition> GetDefinitions() override;
 
-  // Options table: Required for subclasses of Options.
-
-  static lldb_private::OptionDefinition g_option_table[];
-
   // Instance variables to hold the values for command options.
 
   bool m_rsync;
   std::string m_rsync_opts;
   std::string m_rsync_prefix;
   bool m_ignores_remote_hostname;
 
 private:
public:
   lldb_private::Status
   SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
                  ExecutionContext *execution_context) override;
 
   void OptionParsingStarting(ExecutionContext *execution_context) override;
 
   llvm::ArrayRef<OptionDefinition> GetDefinitions() override;
 
-  // Options table: Required for subclasses of Options.
-
-  static lldb_private::OptionDefinition g_option_table[];
-
   // Instance variables to hold the values for command options.
 
   bool m_ssh;
   std::string m_ssh_opts;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(OptionGroupPlatformSSH);
 };
 
 class OptionGroupPlatformCaching : public lldb_private::OptionGroup {
 public:
   OptionGroupPlatformCaching() = default;
 
   ~OptionGroupPlatformCaching() override = default;
 
   lldb_private::Status
   SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
                  ExecutionContext *execution_context) override;
 
   void OptionParsingStarting(ExecutionContext *execution_context) override;
 
   llvm::ArrayRef<OptionDefinition> GetDefinitions() override;
 
-  // Options table: Required for subclasses of Options.
-
-  static lldb_private::OptionDefinition g_option_table[];
-
   // Instance variables to hold the values for command options.
 
   std::string m_cache_dir;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(OptionGroupPlatformCaching);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_Platform_h_

source/Commands/CommandObjectDisassemble.h

public:
     bool frame_line;
     std::string plugin_name;
     std::string flavor_string;
     ArchSpec arch;
     bool some_location_specified; // If no location was specified, we'll select
                                   // "at_pc".  This should be set
     // in SetOptionValue if anything the selects a location is set.
     lldb::addr_t symbol_containing_addr;
-    static OptionDefinition g_option_table[];
   };
 
   CommandObjectDisassemble(CommandInterpreter &interpreter);
 
   ~CommandObjectDisassemble() override;
 
   Options *GetOptions() override { return &m_options; }
 

source/Commands/CommandObjectExpression.h

public:
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override;
 
     Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
                           ExecutionContext *execution_context) override;
 
     void OptionParsingStarting(ExecutionContext *execution_context) override;
 
-    // Options table: Required for subclasses of Options.
-
-    static OptionDefinition g_option_table[];
     bool top_level;
     bool unwind_on_error;
     bool ignore_breakpoints;
     bool allow_jit;
     bool show_types;
     bool show_summary;
     bool debug;
     uint32_t timeout;

tools/driver/Driver.h

public:
     OptionData();
     ~OptionData();
 
     void Clear();
 
     void AddInitialCommand(const char *command, CommandPlacement placement,
                            bool is_file, lldb::SBError &error);
 
-    // static OptionDefinition m_cmd_option_table[];
-
     struct InitialCmdEntry {
       InitialCmdEntry(const char *in_contents, bool in_is_file,
                       bool is_cwd_lldbinit_file_read, bool in_quiet = false)
           : contents(in_contents), is_file(in_is_file),
             is_cwd_lldbinit_file_read(is_cwd_lldbinit_file_read),
             source_quietly(in_quiet) {}
 
       std::string contents;

tools/driver/Driver.cpp

 // added as an atexit handler to make sure we clean them up.
 static void reset_stdin_termios() {
   if (g_old_stdin_termios_is_valid) {
     g_old_stdin_termios_is_valid = false;
     ::tcsetattr(STDIN_FILENO, TCSANOW, &g_old_stdin_termios);
   }
 }
 
 typedef struct {
   uint32_t usage_mask; // Used to mark options that can be used together.  If (1
                        // << n & usage_mask) != 0
                        // then this option belongs to option set n.
   bool required;       // This option is required (in the current usage level)
   const char *long_option; // Full name for this option.
   int short_option;        // Single character for this option.
   int option_has_arg; // no_argument, required_argument or optional_argument
   uint32_t completion_type; // Cookie the option class can use to do define the
                             // argument completion.
   lldb::CommandArgumentType argument_type; // Type of argument this option takes
   const char *usage_text; // Full text explaining what this options does and
                           // what (if any) argument to
                           // pass it.
 } OptionDefinition;
 
 #define LLDB_3_TO_5 LLDB_OPT_SET_3 | LLDB_OPT_SET_4 | LLDB_OPT_SET_5
 #define LLDB_4_TO_5 LLDB_OPT_SET_4 | LLDB_OPT_SET_5
 
-static OptionDefinition g_options[] = {
+static constexpr OptionDefinition g_options[] = {
     {LLDB_OPT_SET_1, true, "help", 'h', no_argument, 0, eArgTypeNone,
      "Prints out the usage information for the LLDB debugger."},
     {LLDB_OPT_SET_2, true, "version", 'v', no_argument, 0, eArgTypeNone,
      "Prints out the current version number of the LLDB debugger."},
     {LLDB_OPT_SET_3, true, "arch", 'a', required_argument, 0,
      eArgTypeArchitecture,
      "Tells the debugger to use the specified architecture when starting and "
      "running the program.  <architecture> must "
static constexpr OptionDefinition g_options[] = {
      "Valid scripting languages that can be specified include Python, Perl, "
      "Ruby and Tcl.  Currently only the Python "
      "extensions have been implemented."},
     {LLDB_3_TO_5, false, "debug", 'd', no_argument, 0, eArgTypeNone,
      "Tells the debugger to print out extra information for debugging itself."},
     {LLDB_OPT_SET_7, true, "repl", 'r', optional_argument, 0, eArgTypeNone,
      "Runs lldb in REPL mode with a stub process."},
     {LLDB_OPT_SET_7, true, "repl-language", 'R', required_argument, 0,
-     eArgTypeNone, "Chooses the language for the REPL."},
-    {0, false, NULL, 0, 0, 0, eArgTypeNone, NULL}};
+     eArgTypeNone, "Chooses the language for the REPL."}};
+
+static constexpr auto g_num_options = sizeof(g_options)/sizeof(OptionDefinition);
 
 static const uint32_t last_option_set_with_args = 2;
 
 Driver::Driver()
     : SBBroadcaster("Driver"), m_debugger(SBDebugger::Create(false)),
       m_option_data() {
   // We want to be able to handle CTRL+D in the terminal to have it terminate
   // certain input
while (end < final_end) {
       sub_len = end - start;
       std::string substring = text_string.substr(start, sub_len);
       fprintf(out, "%*s%s\n", indent, "", substring.c_str());
       start = end + 1;
     }
   }
 }
 
-void ShowUsage(FILE *out, OptionDefinition *option_table,
-               Driver::OptionData data) {
+static void ShowUsage(FILE *out, Driver::OptionData data) {
   uint32_t screen_width = 80;
   uint32_t indent_level = 0;
   const char *name = "lldb";
 
   fprintf(out, "\nUsage:\n\n");
 
   indent_level += 2;
 
   // First, show each usage level set of options, e.g. <cmd>
   // [options-for-level-0]
   //                                                   <cmd>
   //                                                   [options-for-level-1]
   //                                                   etc.
 
-  uint32_t num_options;
   uint32_t num_option_sets = 0;
 
-  for (num_options = 0; option_table[num_options].long_option != NULL;
-       ++num_options) {
-    uint32_t this_usage_mask = option_table[num_options].usage_mask;
+  for (const auto &opt : g_options) {
+    uint32_t this_usage_mask = opt.usage_mask;
     if (this_usage_mask == LLDB_OPT_SET_ALL) {
       if (num_option_sets == 0)
         num_option_sets = 1;
     } else {
       for (uint32_t j = 0; j < LLDB_MAX_NUM_OPTION_SETS; j++) {
         if (this_usage_mask & 1 << j) {
           if (num_option_sets <= j)
             num_option_sets = j + 1;
         }
       }
     }
   }
 
   for (uint32_t opt_set = 0; opt_set < num_option_sets; opt_set++) {
     uint32_t opt_set_mask;
 
     opt_set_mask = 1 << opt_set;
 
     if (opt_set > 0)
       fprintf(out, "\n");
     fprintf(out, "%*s%s", indent_level, "", name);
     bool is_help_line = false;
 
-    for (uint32_t i = 0; i < num_options; ++i) {
-      if (option_table[i].usage_mask & opt_set_mask) {
-        CommandArgumentType arg_type = option_table[i].argument_type;
+    for (const auto &opt : g_options) {
+      if (opt.usage_mask & opt_set_mask) {
+        CommandArgumentType arg_type = opt.argument_type;
         const char *arg_name =
             SBCommandInterpreter::GetArgumentTypeAsCString(arg_type);
         // This is a bit of a hack, but there's no way to say certain options
         // don't have arguments yet...
         // so we do it by hand here.
-        if (option_table[i].short_option == 'h')
+        if (opt.short_option == 'h')
           is_help_line = true;
 
-        if (option_table[i].required) {
-          if (option_table[i].option_has_arg == required_argument)
-            fprintf(out, " -%c <%s>", option_table[i].short_option, arg_name);
-          else if (option_table[i].option_has_arg == optional_argument)
-            fprintf(out, " -%c [<%s>]", option_table[i].short_option, arg_name);
+        if (opt.required) {
+          if (opt.option_has_arg == required_argument)
+            fprintf(out, " -%c <%s>", opt.short_option, arg_name);
+          else if (opt.option_has_arg == optional_argument)
+            fprintf(out, " -%c [<%s>]", opt.short_option, arg_name);
           else
-            fprintf(out, " -%c", option_table[i].short_option);
+            fprintf(out, " -%c", opt.short_option);
         } else {
-          if (option_table[i].option_has_arg == required_argument)
-            fprintf(out, " [-%c <%s>]", option_table[i].short_option, arg_name);
-          else if (option_table[i].option_has_arg == optional_argument)
-            fprintf(out, " [-%c [<%s>]]", option_table[i].short_option,
+          if (opt.option_has_arg == required_argument)
+            fprintf(out, " [-%c <%s>]", opt.short_option, arg_name);
+          else if (opt.option_has_arg == optional_argument)
+            fprintf(out, " [-%c [<%s>]]", opt.short_option,
                     arg_name);
           else
-            fprintf(out, " [-%c]", option_table[i].short_option);
+            fprintf(out, " [-%c]", opt.short_option);
         }
       }
     }
     if (!is_help_line && (opt_set <= last_option_set_with_args))
       fprintf(out, " [[--] <PROGRAM-ARG-1> [<PROGRAM_ARG-2> ...]]");
   }
 
   fprintf(out, "\n\n");
static void ShowUsage(FILE *out, Driver::OptionData data) {
   // more than one usage level, but we only want to print the long form of its
   // information once.
 
   Driver::OptionData::OptionSet options_seen;
   Driver::OptionData::OptionSet::iterator pos;
 
   indent_level += 5;
 
-  for (uint32_t i = 0; i < num_options; ++i) {
+  for (const auto &opt : g_options) {
     // Only print this option if we haven't already seen it.
-    pos = options_seen.find(option_table[i].short_option);
+    pos = options_seen.find(opt.short_option);
     if (pos == options_seen.end()) {
-      CommandArgumentType arg_type = option_table[i].argument_type;
+      CommandArgumentType arg_type = opt.argument_type;
       const char *arg_name =
           SBCommandInterpreter::GetArgumentTypeAsCString(arg_type);
 
-      options_seen.insert(option_table[i].short_option);
-      fprintf(out, "%*s-%c ", indent_level, "", option_table[i].short_option);
+      options_seen.insert(opt.short_option);
+      fprintf(out, "%*s-%c ", indent_level, "", opt.short_option);
       if (arg_type != eArgTypeNone)
         fprintf(out, "<%s>", arg_name);
       fprintf(out, "\n");
-      fprintf(out, "%*s--%s ", indent_level, "", option_table[i].long_option);
+      fprintf(out, "%*s--%s ", indent_level, "", opt.long_option);
       if (arg_type != eArgTypeNone)
         fprintf(out, "<%s>", arg_name);
       fprintf(out, "\n");
       indent_level += 5;
-      OutputFormattedUsageText(out, indent_level, option_table[i].usage_text,
+      OutputFormattedUsageText(out, indent_level, opt.usage_text,
                                screen_width);
       indent_level -= 5;
       fprintf(out, "\n");
     }
   }
 
   indent_level -= 5;
 
fprintf(
       "to be"
       "\n%*sdebugged which means that '%s -- <filename> [<ARG1> [<ARG2>]]' also"
       "\n%*sworks.  But remember to end the options with \"--\" if any of your"
       "\n%*sarguments have a \"-\" in them.\n\n",
       indent_level, "", indent_level, "", name, indent_level, "", indent_level,
       "");
 }
 
-void BuildGetOptTable(OptionDefinition *expanded_option_table,
-                      std::vector<struct option> &getopt_table,
-                      uint32_t num_options) {
-  if (num_options == 0)
+ static void BuildGetOptTable(std::vector<option> &getopt_table) {
+  if (g_num_options == 0)
     return;
 
-  uint32_t i;
-  uint32_t j;
   std::bitset<256> option_seen;
 
-  getopt_table.resize(num_options + 1);
+  getopt_table.resize(g_num_options + 1);
 
-  for (i = 0, j = 0; i < num_options; ++i) {
-    char short_opt = expanded_option_table[i].short_option;
+  uint32_t j = 0;
+  for (const auto &opt : g_options) {
+    char short_opt = opt.short_option;
 
     if (option_seen.test(short_opt) == false) {
-      getopt_table[j].name = expanded_option_table[i].long_option;
-      getopt_table[j].has_arg = expanded_option_table[i].option_has_arg;
+      getopt_table[j].name = opt.long_option;
+      getopt_table[j].has_arg = opt.option_has_arg;
       getopt_table[j].flag = NULL;
-      getopt_table[j].val = expanded_option_table[i].short_option;
+      getopt_table[j].val = opt.short_option;
       option_seen.set(short_opt);
       ++j;
     }
   }
 
   getopt_table[j].name = NULL;
   getopt_table[j].has_arg = 0;
   getopt_table[j].flag = NULL;

 // debugger (i.e. the Command Interpreter) or not.  Return FALSE if the
 // arguments were invalid OR
 // if the user only wanted help or version information.
 
 SBError Driver::ParseArgs(int argc, const char *argv[], FILE *out_fh,
                           bool &exiting) {
   ResetOptionValues();
 
-  SBCommandReturnObject result;
-
   SBError error;
-  std::string option_string;
-  struct option *long_options = NULL;
-  std::vector<struct option> long_options_vector;
-  uint32_t num_options;
-
-  for (num_options = 0; g_options[num_options].long_option != NULL;
-       ++num_options)
-    /* Do Nothing. */;
-
-  if (num_options == 0) {
+  if (g_num_options == 0) {
     if (argc > 1)
       error.SetErrorStringWithFormat("invalid number of options");
     return error;
   }
   
-  BuildGetOptTable(g_options, long_options_vector, num_options);
-
-  if (long_options_vector.empty())
-    long_options = NULL;
-  else
-    long_options = &long_options_vector.front();
-
-  if (long_options == NULL) {
+  std::vector<option> long_options_vector;
+  BuildGetOptTable(long_options_vector);
+  if (long_options_vector.empty()) {
     error.SetErrorStringWithFormat("invalid long options");
     return error;
   }
 
   // Build the option_string argument for call to getopt_long_only.
-
-  for (int i = 0; long_options[i].name != NULL; ++i) {
-    if (long_options[i].flag == NULL) {
-      option_string.push_back((char)long_options[i].val);
-      switch (long_options[i].has_arg) {
+  std::string option_string;
+  auto sentinel_it = std::prev(std::end(long_options_vector));
+  for (auto long_opt_it = long_options_vector.begin();
+            long_opt_it != sentinel_it; ++long_opt_it) {
+    if (long_opt_it->flag == nullptr) {
+      option_string.push_back(static_cast<char>(long_opt_it->val));
+      switch (long_opt_it->has_arg) {
       default:
       case no_argument:
         break;
       case required_argument:
         option_string.push_back(':');
         break;
       case optional_argument:
         option_string.append("::");

 #else
   optreset = 1;
   optind = 1;
 #endif
   int val;
   while (1) {
     int long_options_index = -1;
     val = ::getopt_long_only(argc, const_cast<char **>(argv),
-                             option_string.c_str(), long_options,
+                             option_string.c_str(), long_options_vector.data(),
                              &long_options_index);
 
     if (val == -1)
       break;
     else if (val == '?') {
       m_option_data.m_print_help = true;
       error.SetErrorStringWithFormat("unknown or ambiguous option");
       break;
     } else if (val == 0)
       continue;
     else {
       m_option_data.m_seen_options.insert((char)val);
       if (long_options_index == -1) {
-        for (int i = 0; long_options[i].name || long_options[i].has_arg ||
-                        long_options[i].flag || long_options[i].val;
-             ++i) {
-          if (long_options[i].val == val) {
-            long_options_index = i;
-            break;
-          }
-        }
+        auto long_opt_it = std::find_if(long_options_vector.begin(), sentinel_it,
+            [val](const option& long_option) { return long_option.val == val; });
+        if (long_options_vector.end() != long_opt_it)
+          long_options_index =
+              std::distance(std::begin(long_options_vector), long_opt_it);
       }
 
       if (long_options_index >= 0) {
         const int short_option = g_options[long_options_index].short_option;
 
         switch (short_option) {
         case 'h':
           m_option_data.m_print_help = true;
else {
       }
       if (error.Fail()) {
         return error;
       }
     }
   }
 
   if (error.Fail() || m_option_data.m_print_help) {
-    ShowUsage(out_fh, g_options, m_option_data);
+    ShowUsage(out_fh, m_option_data);
     exiting = true;
   } else if (m_option_data.m_print_version) {
     ::fprintf(out_fh, "%s\n", m_debugger.GetVersionString());
     exiting = true;
   } else if (m_option_data.m_print_python_path) {
     SBFileSpec python_file_spec = SBHostOS::GetLLDBPythonPath();
     if (python_file_spec.IsValid()) {
       char python_path[PATH_MAX];