[Lldb-commits] [PATCH] D122193: Reland "[lldb/test] Add events listener helper class to lldbtest"

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like this implementation a lot. The less threads we use, the better. I have a couple of remarks/questions though:

- given how simple the new approach is, is a dedicated test class really needed? It seems like it should be sufficient to add two utility functions to: create a listener; and fetch an event from it. This would be more flexible, and could be placed next to `lldbutil.expect_state_changes` which is our existing helper function for dealing with state-changed events
- instead of taking a callback, I think it would be simpler if the event retrieval function just returned the event, and let the caller do what ever it deems fit
- `listener.GetNextEvent` will now wait indefinitely if the event is not sent (due to a bug). In this setup I'd replace it with `WaitForEvent(timeout)`, where timeout can be zero if we can guarantee that the event has been sent by the time we call the event retrieval function (this would be ideal as failures would be instantaneous), or a reasonably large value (but less than infinity) if we can't. I think the first option should be possible since that's essentially what `eBroadcastBitStopDiagnosticThread` was doing.

> This reverts commit 8bf893466632cc2597188b431160effcd8cedeef <https://reviews.llvm.org/rG8bf893466632cc2597188b431160effcd8cedeef>

If you hadn't said that I don't think anyone would notice, as the implementation is completely different. :)


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

https://reviews.llvm.org/D122193