mib added inline comments.
================
Comment at: lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp:757
+ bool is_scripted_process = m_process->GetPluginName() == "ScriptedProcess";
for (ThreadSP thread_sp : m_process->Threads()) {
----------------
labath wrote:
> mib wrote:
> > JDevlieghere wrote:
> > > Comparing the plugin name defeats the abstraction a plugin is meant to
> > > provide. While we have other instances of LLDB breaking these
> > > abstractions, I don't recall other places where we compare the plugin
> > > name. The way we normally deal with this is extend the plugins capability
> > > (by adding a method) and implementing it accordingly for all the plugins
> > > (or have a sane default).
> > >
> > > Based on the description of the patch it's not clear to me why this is
> > > special for scripted processes. If we need to special case this I'd like
> > > to see a comment explaining why.
> > I agree, but in this case, `Process::UpdateQueueListIfNeeded` calls the
> > System Runtime plugin `PopulateQueueList` method and since queues are only
> > supported on macOS, it does make sense in my opinion.
> >
> > I think we definitely shouldn't add another system runtime for scripted
> > processes just to support that case, so adding a special case for scripted
> > processes seems to be the less intrusive way to implement it.
> I'm with Jonas here. Even though these Queue concepts are a big mystery do
> me, I would like to understand why the scripted processes do can not go
> through the same code path as "regular" ones.
The reason here why this is not implemented at the Scripted Process plugin
level, is because `UpdateQueueListIfNeeded` is a base method of the `Process`
base class that's not expected to be redefined in the process plugin.
I guess we could either make `UpdateQueueListIfNeeded` virtual and keep the
base class implementation the default and override it in the Scripted Process
plugin class or we can have a `virtual void Process::DoUpdateQueueList() {}`
method that would be called in `Process::UpdateQueueListIfNeeded`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139853/new/
https://reviews.llvm.org/D139853
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
