[lldb][NFC] Refactor printing of short options in help

[DavidSpickett]

Summary

Instead of building a set twice for optional and required,
build a set for each while walking the options once.

Then take advantage of set being sorted meaning we don't
have to enforce the upper/lower order ourselves.

Just cleaned up the formatting on the later loops.
Combined the if conditions and used a single line if.

Depends on D123501

[JDevlieghere]

lldb/source/Interpreter/Options.cpp

457

I'm surprised you change this from !empty() to size(). I personally think the former is more readable. Not an issue in practice, but I do believe the STL guarantees ::empty to be O(1) while technically ::size could be O(n), though I think in practice they're both O(1).

[DavidSpickett]

!empty instead of size

[DavidSpickett]

lldb/source/Interpreter/Options.cpp

457

Fair point. I'll keep to the original style.

[DavidSpickett]

Ping!

[jingham]

Looks like it will do the job, and this version is definitely easier to read.

Were there any tests for the help output? If not, maybe we should add one.

Other than that, LGTM.

[DavidSpickett]

Were there any tests for the help output? If not, maybe we should add one.

For these bits no, in fact I broke them without realising at first. Hence the tests in the other review as you saw, they'll be landed before this.

Thanks for the reviews!

lldb/source/Interpreter/Options.cpp

 //===-- Options.cpp -------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
void Options::GenerateOptionUsage(Stream &strm, CommandObject *cmd,
   // First, show each usage level set of options, e.g. <cmd> [options-for-
   // level-0]
   //                                                   <cmd>
   //                                                   [options-for-level-1]
   //                                                   etc.
 
   uint32_t num_option_sets = GetRequiredOptions().size();
 
-  uint32_t i;
-
   if (!only_print_args) {
     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)
         strm.Printf("\n");
       strm.Indent(name);
 
       // Different option sets may require different args.
       StreamString args_str;
       if (cmd)
         cmd->GetFormattedCommandArguments(args_str, opt_set_mask);
 
       // First go through and print all options that take no arguments as a
       // single string. If a command has "-a" "-b" and "-c", this will show up
       // as [-abc]
 
-      std::set<int> options;
-      std::set<int>::const_iterator options_pos, options_end;
-      for (auto &def : opt_defs) {
-        if (def.usage_mask & opt_set_mask && def.HasShortOption()) {
-          // Add current option to the end of out_stream.
+      // We use a set here so that they will be sorted.
+      std::set<int> required_options;
+      std::set<int> optional_options;
 
-          if (def.required && def.option_has_arg == OptionParser::eNoArgument) {
-            options.insert(def.short_option);
+      for (auto &def : opt_defs) {
+        if (def.usage_mask & opt_set_mask && def.HasShortOption() &&
+            def.option_has_arg == OptionParser::eNoArgument) {
+          if (def.required) {
+            required_options.insert(def.short_option);
+          } else {
+            optional_options.insert(def.short_option);
           }
         }
       }
 
-      if (!options.empty()) {
-        // We have some required options with no arguments
+      if (!required_options.empty()) {
         strm.PutCString(" -");
-        for (i = 0; i < 2; ++i)
-          for (options_pos = options.begin(), options_end = options.end();
-               options_pos != options_end; ++options_pos) {
-            if (i == 0 && ::islower(*options_pos))
-              continue;
-            if (i == 1 && ::isupper(*options_pos))
-              continue;
-            strm << (char)*options_pos;
-          }
+        for (int short_option : required_options)
+          strm.PutChar(short_option);
       }
 
-      options.clear();
-      for (auto &def : opt_defs) {
-        if (def.usage_mask & opt_set_mask && def.HasShortOption()) {
-          // Add current option to the end of out_stream.
-
-          if (!def.required &&
-              def.option_has_arg == OptionParser::eNoArgument) {
-            options.insert(def.short_option);
-          }
-        }
-      }
-
-      if (!options.empty()) {
-        // We have some required options with no arguments
+      if (!optional_options.empty()) {
         strm.PutCString(" [-");
-        for (i = 0; i < 2; ++i)
-          for (options_pos = options.begin(), options_end = options.end();
-               options_pos != options_end; ++options_pos) {
-            if (i == 0 && ::islower(*options_pos))
-              continue;
-            if (i == 1 && ::isupper(*options_pos))
-              continue;
-            strm << (char)*options_pos;
-          }
+        for (int short_option : optional_options)
+          strm.PutChar(short_option);
         strm.PutChar(']');
       }
 
       // First go through and print the required options (list them up front).
-
       for (auto &def : opt_defs) {
-        if (def.usage_mask & opt_set_mask && def.HasShortOption()) {
-          if (def.required && def.option_has_arg != OptionParser::eNoArgument)
-            PrintOption(def, eDisplayBestOption, " ", nullptr, true, strm);
-        }
-      }
+        if (def.usage_mask & opt_set_mask && def.HasShortOption() &&
+            def.required && def.option_has_arg != OptionParser::eNoArgument)
 
       // Now go through again, and this time only print the optional options.
-
       for (auto &def : opt_defs) {
-        if (def.usage_mask & opt_set_mask) {
-          // Add current option to the end of out_stream.
-
-          if (!def.required && def.option_has_arg != OptionParser::eNoArgument)
-            PrintOption(def, eDisplayBestOption, " ", nullptr, true, strm);
-        }
-      }
+        if (def.usage_mask & opt_set_mask && !def.required &&
+            def.option_has_arg != OptionParser::eNoArgument)
 
       if (args_str.GetSize() > 0) {
         if (cmd->WantsRawCommandString() && !only_print_args)
           strm.Printf(" --");
 
         strm << " " << args_str.GetString();
         if (only_print_args)
           break;