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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D66345#1634800 <https://reviews.llvm.org/D66345#1634800>, @teemperor wrote:

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


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.


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

https://reviews.llvm.org/D66345