Options: Reduce code duplication

[labath]

Summary

While investigating breakages caused by D63110, I noticed we were
building the short options strings in three places. Some of them used a
leading ':' to detect missing arguments, and some didn't. This was the
indirect cause of D63110. Here, I move the common code into a utility
function.

Also, unify the code which appends the sentinel value at the end of the
option vector, and make it harder for users to pass invalid argc-argv
combos to getopt (another component of D63110) by having the
OptionParser::Parse function take a (Mutable)ArrayRef.

This unification has uncovered that we don't handle missing arguments
while building aliases, However, it's not possible to write an effective
test for this, as right now it is not possible to return an error out of
the alias parsing code (which means we are printing the generic
"failure" message even after this patch).

[mgorny]

LGTM.

lldb/trunk/include/lldb/Host/OptionParser.h

 //===-- OptionParser.h ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_OptionParser_h_
 #define liblldb_OptionParser_h_
 
 #include <mutex>
 #include <string>
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/ArrayRef.h"
 
 struct option;
 
 namespace lldb_private {
 
 struct OptionDefinition;
 
 struct Option {
   // The definition of the option that this refers to.
   const OptionDefinition *definition;
   // if not NULL, set *flag to val when option found
   int *flag;
   // if flag not NULL, value to set *flag to; else return value
   int val;
 };
 
 class OptionParser {
 public:
   enum OptionArgument { eNoArgument = 0, eRequiredArgument, eOptionalArgument };
 
   static void Prepare(std::unique_lock<std::mutex> &lock);
 
   static void EnableError(bool error);
 
-  static int Parse(int argc, char *const argv[], llvm::StringRef optstring,
-                   const Option *longopts, int *longindex);
+  /// Argv must be an argument vector "as passed to main", i.e. terminated with
+  /// a nullptr.
+  static int Parse(llvm::MutableArrayRef<char *> argv,
+                   llvm::StringRef optstring, const Option *longopts,
+                   int *longindex);
 
   static char *GetOptionArgument();
   static int GetOptionIndex();
   static int GetOptionErrorCause();
   static std::string GetShortOptionString(struct option *long_options);
 };
 }
 
 #endif // liblldb_OptionParser_h_

lldb/trunk/source/Host/common/OptionParser.cpp

 #else
   optreset = 1;
   optind = 1;
 #endif
 }
 
 void OptionParser::EnableError(bool error) { opterr = error ? 1 : 0; }
 
-int OptionParser::Parse(int argc, char *const argv[], llvm::StringRef optstring,
-                        const Option *longopts, int *longindex) {
+int OptionParser::Parse(llvm::MutableArrayRef<char *> argv,
+                        llvm::StringRef optstring, const Option *longopts,
+                        int *longindex) {
   std::vector<option> opts;
   while (longopts->definition != nullptr) {
     option opt;
     opt.flag = longopts->flag;
     opt.val = longopts->val;
     opt.name = longopts->definition->long_option;
     opt.has_arg = longopts->definition->option_has_arg;
     opts.push_back(opt);
     ++longopts;
   }
   opts.push_back(option());
   std::string opt_cstr = optstring;
-  return getopt_long_only(argc, argv, opt_cstr.c_str(), &opts[0], longindex);
+  return getopt_long_only(argv.size() - 1, argv.data(), opt_cstr.c_str(),
+                          &opts[0], longindex);
 }
 
 char *OptionParser::GetOptionArgument() { return optarg; }
 
 int OptionParser::GetOptionIndex() { return optind; }
 
 int OptionParser::GetOptionErrorCause() { return optopt; }
 

lldb/trunk/source/Interpreter/CommandAlias.cpp

static bool ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
   bool success = true;
   OptionArgVector *option_arg_vector = option_arg_vector_sp.get();
 
   if (options_args.size() < 1)
     return true;
 
   Args args(options_args);
   std::string options_string(options_args);
+  // TODO: Find a way to propagate errors in this CommandReturnObject up the
+  // stack.
   CommandReturnObject result;
   // Check to see if the command being aliased can take any command options.
   Options *options = cmd_obj_sp->GetOptions();
   if (options) {
     // See if any options were specified as part of the alias;  if so, handle
     // them appropriately.
     ExecutionContext exe_ctx =
         cmd_obj_sp->GetCommandInterpreter().GetExecutionContext();

lldb/trunk/source/Interpreter/Options.cpp

 // options themselves are never modified, but the API expects a char * anyway,
 // hence the const_cast.
 static std::vector<char *> GetArgvForParsing(const Args &args) {
   std::vector<char *> result;
   // OptionParser always skips the first argument as it is based on getopt().
   result.push_back(const_cast<char *>("<FAKE-ARG0>"));
   for (const Args::ArgEntry &entry : args)
     result.push_back(const_cast<char *>(entry.c_str()));
+  result.push_back(nullptr);
   return result;
 }
 
 // Given a permuted argument, find it's position in the original Args vector.
 static Args::const_iterator FindOriginalIter(const char *arg,
                                              const Args &original) {
   return llvm::find_if(
       original, [arg](const Args::ArgEntry &D) { return D.c_str() == arg; });
for (const auto &entry : llvm::enumerate(args)) {
     if (entry.value().ref.startswith(short_opt) ||
         entry.value().ref.startswith(long_opt))
       return entry.index();
   }
 
   return size_t(-1);
 }
 
-llvm::Expected<Args> Options::ParseAlias(const Args &args,
-                                         OptionArgVector *option_arg_vector,
-                                         std::string &input_line) {
-  StreamString sstr;
-  int i;
-  Option *long_options = GetLongOptions();
+static std::string BuildShortOptions(const Option *long_options) {
+  std::string storage;
+  llvm::raw_string_ostream sstr(storage);
 
-  if (long_options == nullptr) {
-    return llvm::make_error<llvm::StringError>("Invalid long options",
-                                               llvm::inconvertibleErrorCode());
-  }
+  // Leading : tells getopt to return a : for a missing option argument AND to
+  // suppress error messages.
+  sstr << ":";
 
-  for (i = 0; long_options[i].definition != nullptr; ++i) {
+  for (size_t i = 0; long_options[i].definition != nullptr; ++i) {
     if (long_options[i].flag == nullptr) {
       sstr << (char)long_options[i].val;
       switch (long_options[i].definition->option_has_arg) {
       default:
       case OptionParser::eNoArgument:
         break;
       case OptionParser::eRequiredArgument:
         sstr << ":";
         break;
       case OptionParser::eOptionalArgument:
         sstr << "::";
         break;
       }
     }
   }
+  return std::move(sstr.str());
+}
+
+llvm::Expected<Args> Options::ParseAlias(const Args &args,
+                                         OptionArgVector *option_arg_vector,
+                                         std::string &input_line) {
+  Option *long_options = GetLongOptions();
+
+  if (long_options == nullptr) {
+    return llvm::make_error<llvm::StringError>("Invalid long options",
+                                               llvm::inconvertibleErrorCode());
+  }
+
+  std::string short_options = BuildShortOptions(long_options);
 
   Args args_copy = args;
   std::vector<char *> argv = GetArgvForParsing(args);
 
   std::unique_lock<std::mutex> lock;
   OptionParser::Prepare(lock);
   int val;
   while (true) {
     int long_options_index = -1;
-    val = OptionParser::Parse(argv.size(), &*argv.begin(), sstr.GetString(),
-                              long_options, &long_options_index);
+    val = OptionParser::Parse(argv, short_options, long_options,
+                              &long_options_index);
+
+    if (val == ':') {
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "last option requires an argument");
+    }
 
     if (val == -1)
       break;
 
     if (val == '?') {
       return llvm::make_error<llvm::StringError>(
           "Unknown or ambiguous option", llvm::inconvertibleErrorCode());
     }
llvm::Expected<Args> Options::ParseAlias(const Args &args,
   }
 
   return std::move(args_copy);
 }
 
 OptionElementVector Options::ParseForCompletion(const Args &args,
                                                 uint32_t cursor_index) {
   OptionElementVector option_element_vector;
-  StreamString sstr;
   Option *long_options = GetLongOptions();
   option_element_vector.clear();
 
   if (long_options == nullptr)
     return option_element_vector;
 
-  // Leading : tells getopt to return a : for a missing option argument AND to
-  // suppress error messages.
-
-  sstr << ":";
-  for (int i = 0; long_options[i].definition != nullptr; ++i) {
-    if (long_options[i].flag == nullptr) {
-      sstr << (char)long_options[i].val;
-      switch (long_options[i].definition->option_has_arg) {
-      default:
-      case OptionParser::eNoArgument:
-        break;
-      case OptionParser::eRequiredArgument:
-        sstr << ":";
-        break;
-      case OptionParser::eOptionalArgument:
-        sstr << "::";
-        break;
-      }
-    }
-  }
+  std::string short_options = BuildShortOptions(long_options);
 
   std::unique_lock<std::mutex> lock;
   OptionParser::Prepare(lock);
   OptionParser::EnableError(false);
 
   int val;
   auto opt_defs = GetDefinitions();
 
   std::vector<char *> dummy_vec = GetArgvForParsing(args);
 
-  // I stick an element on the end of the input, because if the last element
-  // is option that requires an argument, getopt_long_only will freak out.
-  dummy_vec.push_back(const_cast<char *>("<FAKE-VALUE>"));
-
   bool failed_once = false;
   uint32_t dash_dash_pos = -1;
 
   while (true) {
     bool missing_argument = false;
     int long_options_index = -1;
 
-    val = OptionParser::Parse(dummy_vec.size(), &dummy_vec[0], sstr.GetString(),
-                              long_options, &long_options_index);
+    val = OptionParser::Parse(dummy_vec, short_options, long_options,
+                              &long_options_index);
 
     if (val == -1) {
       // When we're completing a "--" which is the last option on line,
       if (failed_once)
         break;
 
       failed_once = true;
 
OptionElementVector Options::ParseForCompletion(const Args &args,
   }
   return option_element_vector;
 }
 
 llvm::Expected<Args> Options::Parse(const Args &args,
                                     ExecutionContext *execution_context,
                                     lldb::PlatformSP platform_sp,
                                     bool require_validation) {
-  StreamString sstr;
   Status error;
   Option *long_options = GetLongOptions();
   if (long_options == nullptr) {
     return llvm::make_error<llvm::StringError>("Invalid long options.",
                                                llvm::inconvertibleErrorCode());
   }
 
-  // Leading : tells getopt to return a : for a missing option argument AND to
-  // suppress error messages.
-  sstr << ":";
-  for (int i = 0; long_options[i].definition != nullptr; ++i) {
-    if (long_options[i].flag == nullptr) {
-      if (isprint8(long_options[i].val)) {
-        sstr << (char)long_options[i].val;
-        switch (long_options[i].definition->option_has_arg) {
-        default:
-        case OptionParser::eNoArgument:
-          break;
-        case OptionParser::eRequiredArgument:
-          sstr << ':';
-          break;
-        case OptionParser::eOptionalArgument:
-          sstr << "::";
-          break;
-        }
-      }
-    }
-  }
+  std::string short_options = BuildShortOptions(long_options);
   std::vector<char *> argv = GetArgvForParsing(args);
-  // If the last option requires an argument but doesn't have one,
-  // some implementations of getopt_long will still try to read it.
-  argv.push_back(nullptr);
   std::unique_lock<std::mutex> lock;
   OptionParser::Prepare(lock);
   int val;
   while (true) {
     int long_options_index = -1;
-    val = OptionParser::Parse(argv.size() - 1, &*argv.begin(), sstr.GetString(),
-                              long_options, &long_options_index);
+    val = OptionParser::Parse(argv, short_options, long_options,
+                              &long_options_index);
 
     if (val == ':') {
       error.SetErrorStringWithFormat("last option requires an argument");
       break;
     }
 
     if (val == -1)
       break;