Page MenuHomePhabricator

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

Authored by teemperor on Aug 16 2019, 6:35 AM.

Details

Reviewers
labath
JDevlieghere
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

teemperor created this revision.Aug 16 2019, 6:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2019, 6:35 AM
teemperor updated this revision to Diff 215586.Aug 16 2019, 6:41 AM
  • Fixed some minor issues.
labath accepted this revision.Aug 16 2019, 6:56 AM
labath added a subscriber: 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
523–526

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).

This revision is now accepted and ready to land.Aug 16 2019, 6:56 AM
teemperor updated this revision to Diff 215595.Aug 16 2019, 7:25 AM

Thanks!

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

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 added a subscriber: 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 added inline comments.
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
515

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 closed this revision.Aug 19 2019, 12:45 AM
teemperor marked 5 inline comments as done.

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
515

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

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.

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.

This comment was removed by jingham.