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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
