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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139853