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

[Pavel Labath via Phabricator via lldb-commits]

labath 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()) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139853