[Lldb-commits] [PATCH] D66345: [lldb][NFC] Allow for-range iterating over StringList

[Raphael Isemann via Phabricator via lldb-commits]

teemperor closed this revision.
teemperor marked 5 inline comments as done.
teemperor added a comment.

In D66345#1633172 <https://reviews.llvm.org/D66345#1633172>, @labath wrote:

> In D66345#1633118 <https://reviews.llvm.org/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 :).



================
Comment at: lldb/include/lldb/Utility/StringList.h:26
 class StringList {
+  typedef std::vector<std::string> StorageType;
+
----------------
JDevlieghere wrote:
> 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
Thanks!


================
Comment at: lldb/source/Commands/CommandObjectApropos.cpp:70
+            max_len = std::max(max_len, command.size());
           }
 
----------------
amccarth wrote:
> Or
> 
> ```
> const size_t max_len =
>     std::max_element(commands_found.begin(), commands_found.end());
> ```
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.


================
Comment at: lldb/unittests/Utility/StringListTest.cpp:515
+
+TEST(StringListTest, ForRangeSingle) {
+  StringList s;
----------------
amccarth wrote:
> 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.
Yeah, that was originally just for one element (before I just made one test with three elements which should be enough coverage).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66345/new/

https://reviews.llvm.org/D66345