[Lldb-commits] [PATCH] D61921: [Target] Generalize language-specific behavior in ThreadPlanStepThrough

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

In D61921#1503282 <https://reviews.llvm.org/D61921#1503282>, @jingham wrote:

> In D61921#1502502 <https://reviews.llvm.org/D61921#1502502>, @labath wrote:
>
> > If this iteration is going to be used a lot, I'd recommend taking a bit of time to implement an iterator abstraction over the language runtimes. It takes a bit longer to set up, but I hope we can all agree that `for (runtime: process->GetLanguageRuntimes()) runtime->foo();` is more readable than `process->ForEachLanguageRuntime([] (runtime) { runtime->foo(); })`. This is particularly true if you need some sort of a control flow construct (`continue`, `break`, `return`) in the loop "body".
>
>
> +1


Yeah this seems reasonable.

In D61921#1503290 <https://reviews.llvm.org/D61921#1503290>, @jingham wrote:

> On macOS, there are a handful of runtimes that the Plugin Manager knows about - C++, ObjC, maybe Swift.  But, for instance, we only load the ObjC language runtime into the process' m_language_runtimes array when we see libobjc.dylib get loaded.  A pure C++ program might never load libobjc.dylib, and so even though the Plugin Manager knows we have support for the ObjC language runtime, that plugin wouldn't be active in the current lldb_private::Process.  So there's a real difference between the "available" and the "currently loaded" runtimes. I was saying I didn't see any compelling reason to have an iterator over the available runtimes, just over the loaded ones.  Not that we didn't need one or the other iterator.


Okay, that makes sense to me. Thanks for clearing that up. If I understand correctly (which I think I do), creating an iterator abstraction to iterate over the process' currently loaded runtimes instead of over every available runtime is the better option here. I'll follow up with that change later today or tomorrow unless somebody believes this is the wrong idea.


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

https://reviews.llvm.org/D61921