[Lldb-commits] [PATCH] D157556: Replace the singleton "ShadowListener" with a primary and N secondary listeners

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
Comment at: lldb/include/lldb/Target/Process.h:348-357
     eBroadcastBitStateChanged = (1 << 0),
     eBroadcastBitInterrupt = (1 << 1),
     eBroadcastBitSTDOUT = (1 << 2),
     eBroadcastBitSTDERR = (1 << 3),
     eBroadcastBitProfileData = (1 << 4),
     eBroadcastBitStructuredData = (1 << 5),
   };
----------------
I wonder if it might be better to add a new element to the enum, something like `eBroadcastBitAll = eBroadcastBitStageChanged | ... | eBroadcastBitStructuredData,`.

If we have to add a new element to this enumeration, I'm not sure everyone will realize that `g_all_event_bits` needs to be updated in a separate file (even though it's right below the enum definition).


================
Comment at: lldb/include/lldb/Target/Process.h:616-619
+  void SetShadowListener(lldb::ListenerSP shadow_listener_sp) {
+    if (shadow_listener_sp)
+      AddListener(shadow_listener_sp, g_all_event_bits);
+  }
----------------
Why was this moved down?


================
Comment at: lldb/source/Utility/Broadcaster.cpp:246
+    // listeners.
+    if (!is_hijack) {
+      for (auto &pair : GetListeners()) {
----------------
mib wrote:
> Then, as a follow-up to my suggestion, you could check `hijacking_listener_sp ` instead of checking the boolean.
Instead of having the bool flag for `is_hijack`, you can compare `primary_listener_sp` against `hijacking_listener_sp`.

Then the setup above gets less complex, like so:
```
ListenerSP primary_listener_sp = hijacking_listener_sp;
if (!primary_listener_sp)
  primary_listener_sp = m_primary_listener_sp;
```


================
Comment at: lldb/source/Utility/Broadcaster.cpp:247
+    if (!is_hijack) {
+      for (auto &pair : GetListeners()) {
+        if (!(pair.second & event_type))
----------------
I wonder if it might be useful to introduce a variant of `GetListeners` which takes an `event_type` and only gives you back the ones you want. That would simplify this loop (and the one in the other branching path). You could also just pass that vector along to the `Event` instead of filtering and adding them one at a time (so you're not paying for potential vector resizing costs).


================
Comment at: lldb/source/Utility/Broadcaster.cpp:253
+          continue;
+        if (pair.first != hijacking_listener_sp 
+            && pair.first != m_primary_listener_sp)
----------------
nit: Checking to see if `pair.first` is `hijacking_listener_sp` is redundant. If you've gotten to this point, `hijacking_listener_sp` must be pointing to `nullptr`.


================
Comment at: lldb/source/Utility/Broadcaster.cpp:254
+        if (pair.first != hijacking_listener_sp 
+            && pair.first != m_primary_listener_sp)
+          event_sp->AddPendingListener(pair.first);
----------------
Why might `pair.first` be `m_primary_listener_sp`? I would assume that if a primary listener is set, it wouldn't be among the other listeners in a broadcaster.... But I suppose nothing stops you from doing that.


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:26
     @skipUnlessDarwin
-    @skipIfDarwin
+    #@skipIfDarwin
     def test_passthrough_launch(self):
----------------
Remove this comment?


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:38-44
+            self, self.dbg.GetListener(), self.mux_process.GetBroadcaster()
+        )
+        self.assertState(lldb.SBProcess.GetStateFromEvent(event), lldb.eStateRunning)
+        event = lldbutil.fetch_next_event(
+            self, self.dbg.GetListener(), self.mux_process.GetBroadcaster()
+        )
+        self.assertState(lldb.SBProcess.GetStateFromEvent(event), lldb.eStateStopped)
----------------
This is to make sure that the primary listener is getting the run/stop events before the `mux_process_listener` right?


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:56
     @skipUnlessDarwin
-    @skipIfDarwin
+    #@skipIfDarwin
     def test_multiplexed_launch(self):
----------------
delete?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157556