[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

Thu Aug 18 12:10:03 PDT 2022

mgorny marked an inline comment as done.
mgorny added a comment.

In D131160#3732668 <https://reviews.llvm.org/D131160#3732668>, @labath wrote:

> Thanks for taking care of the posix part.
>
> I'm sort of warming up to the idea of reusing pending callback machinery for this. Besides avoiding the awkward naming problem, it also avoid another somewhat strange situation where our EventHandle offer additional actions on the event (which is different from all our other handle types, which only support deletion). All it would take is to add some locking around the PendingCallback manipulations, and write to the pipe (or trigger the event) whenever a new callback is registered, so that the Run function can invoke it.

To be honest, I don't have a strong opinion, so I leave the decision to you. I was also thinking about this but decided to avoid changing the paradigm while rebasing/finishing the existing code, especially that I can't test it on Windows. But I can do it if you like.

> It's a slightly different programming paradigm, and I suppose it also depends on which mechanism would be more natural for our InterruptRead use case (have you tried implementing it using this?), but I think the two mechanisms should be equivalent (as in, one could implement one using the other -- fairly easily).

Actually, I've started experimenting on rewriting the read thread option in `Communication` using this (with the goal of using the thread both for reads and writes).

================
Comment at: lldb/source/Host/posix/MainLoopPosix.cpp:230
+  const int event_pipe_fd = m_event_pipe.GetReadFileDescriptor();
+  m_read_fds.insert({event_pipe_fd, [this, event_pipe_fd](MainLoopBase &loop) {
+                       char c;
----------------
labath wrote:
> I like this.
Thanks. I definitely like it more than doing it three times too ;-).

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

https://reviews.llvm.org/D131160