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: > > > 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. 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
