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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
