[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

[Michał Górny via Phabricator via lldb-commits]

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