labath added a comment. In D122193#3399921 <https://reviews.llvm.org/D122193#3399921>, @mib wrote:
> 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 really related to state > change (regarding where this should go). They're not related to state change, but they are related to events. My point was simply that it'd be nice to have event-handling code grouped together. >> - 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 the check > themselves by returning the event ? Yes, that's the general idea. >> - `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 ? The test suite timeout is rather large and relies on the an optional python module. I think it would be better to have a separate timeout here, which would also give a better error message The function can just fail if the event retrieval times out. Most of the lldbutil methods achieve that by taking an `test` object so they can call some unittest methods on (`fail(msg)` would probably be appropriate here). 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
