mgorny added inline comments.
================ Comment at: lldb/source/Core/Communication.cpp:427 // Notify the read thread. - m_connection_sp->InterruptRead(); ---------------- labath wrote: > mgorny wrote: > > labath wrote: > > > mgorny wrote: > > > > labath wrote: > > > > > Have you considered putting this code (some version of it) inside > > > > > `InterruptRead`? Basically replacing the `select` call inside > > > > > BytesAvailable with something MainLoop-based ? > > > > To be honest, I've been considering removing `InterruptRead()` > > > > entirely, now that the read loop is triggered by available input rather > > > > than read-with-timeout. However, it's still used by editline support. > > > > > > > > That said, I'm not sure what's your idea, given that `Connection` does > > > > not have awareness of `Communication` that's using it. I suppose I > > > > could pass the `MainLoop` instance as a parameter but we'd still have > > > > to maintain a separate version for editline support, and we only have a > > > > single caller here. > > > > To be honest, I've been considering removing `InterruptRead()` > > > > entirely, now that the read loop is triggered by available input rather > > > > than read-with-timeout. However, it's still used by editline support. > > > If we could do that, then it would be even better. And it looks like it > > > should be possible to use a MainLoop instance inside the Editline class, > > > instead of the built-in interruption support. > > > > > > > That said, I'm not sure what's your idea, given that `Connection` does > > > > not have awareness of `Communication` that's using it. I suppose I > > > > could pass the `MainLoop` instance as a parameter but we'd still have > > > > to maintain a separate version for editline support, and we only have a > > > > single caller here. > > > > > > I don't claim to have thought this out completely, but I was imagining > > > that the MainLoop instance would be internal to the Connection class. The > > > external interface of the Connection class would remain unchanged (so the > > > Communication class would not need to change), and the InterruptRead > > > function would be implemented using the MainLoop functionality. > > > > That said, I'm not sure what's your idea, given that `Connection` does > > > > not have awareness of `Communication` that's using it. I suppose I > > > > could pass the `MainLoop` instance as a parameter but we'd still have > > > > to maintain a separate version for editline support, and we only have a > > > > single caller here. > > > > > > I don't claim to have thought this out completely, but I was imagining > > > that the MainLoop instance would be internal to the Connection class. The > > > external interface of the Connection class would remain unchanged (so the > > > Communication class would not need to change), and the InterruptRead > > > function would be implemented using the MainLoop functionality. > > > > So basically make `ConnectionFileDescriptor::BytesAvailable()` use a main > > loop to wait for some event, and make > > `ConnectionFileDescriptor::InterruptRead()` interrupt that? > > > > I suppose both options are feasible but I don't have sufficient foresight > > to tell which one is better. That said, I have no clue about > > `ConnectionGenericFileWindows` and I feel that using `MainLoop` would make > > it possible to reduce code duplication between it and CFD. Though I'd > > preferred that someone with access to a Windows environment done that. > > So basically make `ConnectionFileDescriptor::BytesAvailable()` use a main > > loop to wait for some event, and make > > `ConnectionFileDescriptor::InterruptRead()` interrupt that? > > Correct. > > > I suppose both options are feasible but I don't have sufficient foresight > > to tell which one is better. That said, I have no clue about > > `ConnectionGenericFileWindows` and I feel that using `MainLoop` would make > > it possible to reduce code duplication between it and CFD. Though I'd > > preferred that someone with access to a Windows environment done that. > > That is my hope as well, but it's not going to be very easy, since the > ReadFile (used by CGFW) api requires you to actually attempt to read some > data in order to wait for the result) . That said, I'm not sure this really > speaks in favour of one approach over the other. If e.g. we are able to use > the same MainLoop code for both CFD and CGFW, then we could probably move > that code over to a common base class. > > What I would like to avoid is to increase the number of places that do > selects. So, if you can port editline code to something mainloop-based and > remove the `InterruptRead`, then that's fine by me, and we can figure out > what to do with windows later. > What I would like to avoid is to increase the number of places that do > selects. So, if you can port editline code to something mainloop-based and > remove the `InterruptRead`, then that's fine by me, and we can figure out > what to do with windows later. Well, I suppose I'll add that to my TODO and maybe I'll manage to get to it before I retire ;-). Also note that the proposed gdb-remote interrupt changes rely on `InterruptRead()`, so I'd have to change that as well. However, I think it's better to focus on multiprocess first, as that definitely will influence the final shape of how we do async there. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132283/new/ https://reviews.llvm.org/D132283 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
