[lldb][NFC] Allow for-range iterating over StringList

[teemperor]

Summary

Added support for for-range iterating over StringList. There is no reverse-iterator support because it seems
no one is doing that in LLDB. Also added some tests and migrated LLDB code over to for-range loops
where possible.

[teemperor]

• Fixed some minor issues.

[labath]

Definitely an improvement, though one day, i'd like to get rid of the StringList class (and various other XXXList classes) completely :)

lldb/unittests/Utility/StringListTest.cpp

522–525

maybe EXPECT_THAT(recorded, testing::ElementsAre("a", "b", "c")) ?
(The advantages are that it's shorter, produces better error messages, and doesn't cause the subsequent checks to invoke UB if recorded.size happens to be less than 3).

[teemperor]

Thanks!

Not sure if we can get rid of StringList so easily as we still have SBStringList.

[labath]

In D66345#1633118, @teemperor wrote:

Not sure if we can get rid of StringList so easily as we still have SBStringList.

We can keep the SBStringList. We can just have it be backed by a vector<string> instead of the StringList thingy...

[JDevlieghere]

LGTM with the inline comment.

lldb/include/lldb/Utility/StringList.h

26

This typedef is commonly collection in LLDB [1]. I think we should do the same here for consistency.

[1] See TypeMap.h, Value.h, QueueList.h etc

[amccarth]

lldb/source/Commands/CommandObjectApropos.cpp

70

Or

const size_t max_len =
    std::max_element(commands_found.begin(), commands_found.end());

lldb/source/Commands/CommandObjectType.cpp

439

Constructing the const_type_name isn't necessary anymore. Just change the if to:

if (!type_name.empty())

lldb/source/Utility/StringList.cpp

67

Optionally add:

m_strings.reserve(m_strings.size() + strings.GetSize());

lldb/unittests/Utility/StringListTest.cpp

514

What does "ForRangeSingle" mean here? The name led me to believe you were going to test a 1-element StringList, but I see three elements.

[teemperor]

In D66345#1633172, @labath wrote:

In D66345#1633118, @teemperor wrote:

Not sure if we can get rid of StringList so easily as we still have SBStringList.

We can keep the SBStringList. We can just have it be backed by a vector<string> instead of the StringList thingy...

I rarely touch the SB* classes, but I always thought we just want them as thin wrappers around LLDB classes? If not, then yeah, removing StringList seems good to me :).

lldb/include/lldb/Utility/StringList.h

26

Thanks!

lldb/source/Commands/CommandObjectApropos.cpp

70

Don't think that works without a lambda that calls .size(), but StringList actually has a method for finding the max string length so we might as well use that.

lldb/unittests/Utility/StringListTest.cpp

514

Yeah, that was originally just for one element (before I just made one test with three elements which should be enough coverage).

[labath]

In D66345#1634800, @teemperor wrote:

In D66345#1633172, @labath wrote:

In D66345#1633118, @teemperor wrote:

Not sure if we can get rid of StringList so easily as we still have SBStringList.

We can keep the SBStringList. We can just have it be backed by a vector<string> instead of the StringList thingy...

I rarely touch the SB* classes, but I always thought we just want them as thin wrappers around LLDB classes? If not, then yeah, removing StringList seems good to me :).

Yes, the SB classes should be thin, but I would interpret the "thinness" as "they shouldn't implement non-trivial logic", not as "there must be a non-SB class of the same name which they delegate to". I don't think the latter would be fair because the SB classes are basically frozen solid, and the second requirement would inflict that solidity on the rest of lldb too. We should have the ability refactor, clean up or otherwise improve internal lldb interfaces even if that means diverging the SB and non-SB class variants. Now, that divergence has a cost, and that should be weighed against the other benefits of the change, but it shouldn't be an immediate show-stopper. In this case, I would say that the cost of that is quite low, because StringList essentially is a std::vector<std::string> and anything extra it provides on top of that (e.g. LongestCommonPrefix) can be easily implemented as a free function.

[clayborg]

The other thing to think about is the SB layer must be thread safe. StringList is not thread safe right now, but it probably should be. We might need to keep the internal version of StringList and make sure it is thread safe. That will ensure that our buildbots always run clean.

[jingham]

This comment was removed by jingham.

lldb/include/lldb/Utility/CompletionRequest.h

public:
   }
 
   /// Adds multiple possible completion strings.
   ///
   /// \param completions The list of completions.
   ///
   /// \see AddCompletion
   void AddCompletions(const StringList &completions) {
-    for (std::size_t i = 0; i < completions.GetSize(); ++i)
-      AddCompletion(completions.GetStringAtIndex(i));
+    for (const std::string &completion : completions)
+      AddCompletion(completion);
   }
 
   /// Adds multiple possible completion strings alongside their descriptions.
   ///
   /// The number of completions and descriptions must be identical.
   ///
   /// \param completions The list of completions.
   /// \param completions The list of descriptions.

lldb/include/lldb/Utility/StringList.h

 namespace lldb_private {
 class Log;
 class Stream;
 }
 
 namespace lldb_private {
 
 class StringList {
+  typedef std::vector<std::string> StorageType;
+
 public:
   StringList();
 
   explicit StringList(const char *str);
 
   StringList(const char **strv, int strc);
 
   virtual ~StringList();
public:
   void AppendList(StringList strings);
 
   size_t GetSize() const;
 
   void SetSize(size_t n) { m_strings.resize(n); }
 
   size_t GetMaxStringLength() const;
 
+  typedef StorageType::iterator iterator;
+  typedef StorageType::const_iterator const_iterator;
+
+  iterator begin() { return m_strings.begin(); }
+  iterator end() { return m_strings.end(); }
+  const_iterator begin() const { return m_strings.begin(); }
+  const_iterator end() const { return m_strings.end(); }
+
   std::string &operator[](size_t idx) {
     // No bounds checking, verify "idx" is good prior to calling this function
     return m_strings[idx];
   }
 
   const std::string &operator[](size_t idx) const {
     // No bounds checking, verify "idx" is good prior to calling this function
     return m_strings[idx];

lldb/source/Breakpoint/WatchpointOptions.cpp

if (level == eDescriptionLevelBrief) {
     return;
   }
 
   s->IndentMore();
   s->Indent("watchpoint commands:\n");
 
   s->IndentMore();
   if (data && data->user_source.GetSize() > 0) {
-    const size_t num_strings = data->user_source.GetSize();
-    for (size_t i = 0; i < num_strings; ++i) {
-      s->Indent(data->user_source.GetStringAtIndex(i));
+    for (const std::string &line : data->user_source) {
+      s->Indent(line);
       s->EOL();
     }
   } else {
     s->PutCString("No commands.\n");
   }
   s->IndentLess();
   s->IndentLess();
 }

lldb/source/Commands/CommandObjectApropos.cpp

if (!search_word.empty()) {
                                        "debugger commands.\n",
                                        args[0].c_str());
       } else {
         if (commands_found.GetSize() > 0) {
           result.AppendMessageWithFormat(
               "The following commands may relate to '%s':\n", args[0].c_str());
           size_t max_len = 0;
 
-          for (size_t i = 0; i < commands_found.GetSize(); ++i) {
-            size_t len = strlen(commands_found.GetStringAtIndex(i));
-            if (len > max_len)
-              max_len = len;
+          for (const std::string &command : commands_found) {
+            max_len = std::max(max_len, command.size());
           }
 
           for (size_t i = 0; i < commands_found.GetSize(); ++i)
             m_interpreter.OutputFormattedHelpText(
                 result.GetOutputStream(), commands_found.GetStringAtIndex(i),
                 "--", commands_help.GetStringAtIndex(i), max_len);
         }
       }
 

lldb/source/Commands/CommandObjectCommands.cpp

protected:
   }
 
   void IOHandlerInputComplete(IOHandler &io_handler,
                               std::string &data) override {
     io_handler.SetIsDone(true);
     if (m_regex_cmd_up) {
       StringList lines;
       if (lines.SplitIntoLines(data)) {
-        const size_t num_lines = lines.GetSize();
         bool check_only = false;
-        for (size_t i = 0; i < num_lines; ++i) {
-          llvm::StringRef bytes_strref(lines[i]);
-          Status error = AppendRegexSubstitution(bytes_strref, check_only);
+        for (const std::string &line : lines) {
+          Status error = AppendRegexSubstitution(line, check_only);
           if (error.Fail()) {
             if (!GetDebugger().GetCommandInterpreter().GetBatchCommandMode()) {
               StreamSP out_stream = GetDebugger().GetAsyncOutputStream();
               out_stream->Printf("error: %s\n", error.AsCString());
             }
           }
         }
       }

lldb/source/Commands/CommandObjectMultiword.cpp

bool CommandObjectMultiword::Execute(const char *args_string,
   error_msg.append("'");
   error_msg.append(GetCommandName());
   error_msg.append(" ");
   error_msg.append(sub_command);
   error_msg.append("'.");
 
   if (num_subcmd_matches > 0) {
     error_msg.append(" Possible completions:");
-    for (size_t i = 0; i < matches.GetSize(); i++) {
+    for (const std::string &match : matches) {
       error_msg.append("\n\t");
-      error_msg.append(matches.GetStringAtIndex(i));
+      error_msg.append(match);
     }
   }
   error_msg.append("\n");
   result.AppendRawError(error_msg.c_str());
   result.SetStatus(eReturnStatusFailed);
   return false;
 }
 

lldb/source/Commands/CommandObjectType.cpp

if (interpreter) {
 
                 TypeSummaryImplSP script_format;
                 script_format = std::make_shared<ScriptSummaryFormat>(
                     options->m_flags, funct_name_str.c_str(),
                     lines.CopyList("    ").c_str());
 
                 Status error;
 
-                for (size_t i = 0; i < options->m_target_types.GetSize(); i++) {
-                  const char *type_name =
-                      options->m_target_types.GetStringAtIndex(i);
+                for (const std::string &type_name : options->m_target_types) {
                   CommandObjectTypeSummaryAdd::AddSummary(
                       ConstString(type_name), script_format,
                       (options->m_regex
                            ? CommandObjectTypeSummaryAdd::eRegexSummary
                            : CommandObjectTypeSummaryAdd::eRegularSummary),
                       options->m_category, &error);
                   if (error.Fail()) {
                     error_sp->Printf("error: %s", error.AsCString());
if (interpreter) {
                     class_name_str.c_str());
 
                 lldb::TypeCategoryImplSP category;
                 DataVisualization::Categories::GetCategory(
                     ConstString(options->m_category.c_str()), category);
 
                 Status error;
 
-                for (size_t i = 0; i < options->m_target_types.GetSize(); i++) {
-                  const char *type_name =
-                      options->m_target_types.GetStringAtIndex(i);
+                for (const std::string &type_name : options->m_target_types) {
                   ConstString const_type_name(type_name);
                   if (const_type_name) {
                     if (!CommandObjectTypeSynthAdd::AddSynth(
                             const_type_name, synth_provider,
                             options->m_regex
                                 ? CommandObjectTypeSynthAdd::eRegexSynth
                                 : CommandObjectTypeSynthAdd::eRegularSynth,
                             options->m_category, &error)) {
                       error_sp->Printf("error: %s\n", error.AsCString());

lldb/source/Utility/Args.cpp

 }
 
 // Args constructor
 Args::Args(llvm::StringRef command) { SetCommandString(command); }
 
 Args::Args(const Args &rhs) { *this = rhs; }
 
 Args::Args(const StringList &list) : Args() {
-  for (size_t i = 0; i < list.GetSize(); ++i)
-    AppendArgument(list[i]);
+  for (const std::string &arg : list)
+    AppendArgument(arg);
 }
 
 Args &Args::operator=(const Args &rhs) {
   Clear();
 
   m_argv.clear();
   m_entries.clear();
   for (auto &entry : rhs.m_entries) {

lldb/source/Utility/StringList.cpp

 void StringList::AppendList(const char **strv, int strc) {
   for (int i = 0; i < strc; ++i) {
     if (strv[i])
       m_strings.push_back(strv[i]);
   }
 }
 
 void StringList::AppendList(StringList strings) {
-  size_t len = strings.GetSize();
-
-  for (size_t i = 0; i < len; ++i)
-    m_strings.push_back(strings.GetStringAtIndex(i));
+  m_strings.insert(m_strings.end(), strings.begin(), strings.end());
 }
 
 size_t StringList::GetSize() const { return m_strings.size(); }
 
 size_t StringList::GetMaxStringLength() const {
   size_t max_length = 0;
   for (const auto &s : m_strings) {
     const size_t len = s.size();

lldb/unittests/Editline/EditlineTest.cpp

 
 bool EditlineAdapter::IsInputComplete(lldb_private::Editline *editline,
                                       lldb_private::StringList &lines,
                                       void *baton) {
   // We'll call ourselves complete if we've received a balanced set of braces.
   int start_block_count = 0;
   int brace_balance = 0;
 
-  for (size_t i = 0; i < lines.GetSize(); ++i) {
-    for (auto ch : lines[i]) {
+  for (const std::string &line : lines) {
+    for (auto ch : line) {
       if (ch == '{') {
         ++start_block_count;
         ++brace_balance;
       } else if (ch == '}')
         --brace_balance;
     }
   }
 
TEST_F(EditlineTestFixture, EditlineReceivesMultiLineText) {
 
   EXPECT_TRUE(GetEditlineAdapter().GetLines(el_reported_lines,
                                             input_interrupted, TIMEOUT_MILLIS));
   EXPECT_FALSE(input_interrupted);
 
   // Without any auto indentation support, our output should directly match our
   // input.
   std::vector<std::string> reported_lines;
-  for (size_t i = 0; i < el_reported_lines.GetSize(); ++i)
-    reported_lines.push_back(el_reported_lines[i]);
+  for (const std::string &line : el_reported_lines)
+    reported_lines.push_back(line);
 
   EXPECT_THAT(reported_lines, testing::ContainerEq(input_lines));
 }
 
 #endif

lldb/unittests/Utility/StringListTest.cpp

TEST(StringListTest, GetMaxStringLengthMixed) {
   s.AppendString("1");
   EXPECT_EQ(4U, s.GetMaxStringLength());
 }
 
 TEST(StringListTest, GetMaxStringLengthEmpty) {
   StringList s;
   EXPECT_EQ(0U, s.GetMaxStringLength());
 }
+
+TEST(StringListTest, ForRangeEmpty) {
+  StringList s;
+  for (const std::string &e : s)
+    FAIL() << "Shouldn't have hit an element in for range";
+}
+
+TEST(StringListTest, ForRangeSingle) {
+  StringList s;
+  s.AppendString("a");
+  s.AppendString("b");
+  s.AppendString("c");
+  std::vector<std::string> recorded;
+  for (const std::string &e : s)
+    recorded.push_back(e);
+  EXPECT_EQ(3, recorded.size());
+  EXPECT_EQ("a", recorded.at(0));
+  EXPECT_EQ("b", recorded.at(1));
+  EXPECT_EQ("c", recorded.at(2));
+}