[Lldb-commits] [PATCH] D139853: [lldb/Process] Populate queues in Scripted Process

[Med Ismail Bennani via Phabricator via lldb-commits]

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