mib added a comment. Hey @labath, thanks for the feedback! Here are some other questions ?
In D122193#3399109 <https://reviews.llvm.org/D122193#3399109>, @labath wrote: > 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 I think we could replace the class by a function. Also, at the moment, I'm not sure the tested events (progress/diagnostic) are related to state change. > - 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 Should we remove the `assertEvent` method and let the users handle it themselves by returning the event ? > - `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. I expected to use the test suite timeout to stop the infinite loop, but with this suggestion and the previous one, what should the function return after timing out ? >> 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. :) Right :p but I wanted to keep the history CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122193/new/ https://reviews.llvm.org/D122193 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
