Improve hello_debugger - #20
Conversation
Make it so that the debugger is actually looping through all lines while in running, so that pause stops on whatever current line it's on, or setting a breakpoint will eventually hit. Also fixed breakpoint verified state for the last line, as line numbers are 1-based.
| Debugger(const EventHandler&); | ||
|
|
||
| // run() instructs the debugger to continue execution. | ||
| // runs the debugger's main loop |
There was a problem hiding this comment.
nit: The documentation pattern I've been using for functions / methods is verb() does blah...
Also super-dooper-uber-nit, fullstops.
| } | ||
|
|
||
| void Debugger::resume() { | ||
| running = true; |
There was a problem hiding this comment.
Although you're using an atomic, this is racy. running can be set to false at line 143.
There was a problem hiding this comment.
I'm not sure I see the problem here. If a separate thread were to set running to false at line 143 at the same time as another thread sets it to true here, things still remain consistent. In effect, hitting a breakpoint and pressing continue at the same time would mean one of them would win. In reality, the UI wouldn't let you press continue unless you were already paused, so it should be fine.
Maybe I'm not seeing it, though. What would you suggest here?
There was a problem hiding this comment.
My concern is that whenever I see the usage of std::atomic<bool> as signals there's usually a race condition. Here there is, but it looks like it'll recover.
However, this is example code, and I'm not sure we should be demonstrating patterns that can hide raciness.
Ideally in this sort of set up, I like to use a thread-safe queue of events for controlling the worker, and a query API for current state. This seems overkill for what we have here. Instead, we can adapt the Event a bit, and still keep things synchronized with a mutex lock: ben-clayton@3027fc0
What do you think?
There was a problem hiding this comment.
Instead, we can adapt the Event a bit, and still keep things synchronized with a mutex lock: ben-clayton/cppdap@3027fc0
What do you think?
Yes, I like this a lot more. I think it 'test' should be called 'test_and_reset' to make it clear that it resets the fire state, but yeah, your solution of using two events is much better. In fact, originally I had wanted to query to state of the "canRun" event, but saw that it didn't have that, and figured an atomic would do the same; but it's less clean, as you pointed out.
EDIT: have you tested it? I'll give it a go later to see if it behaves as expected. Am a bit concerned with the sleep(100ms) in there, as it may slow down stepping. I think we only want to do that when we're not paused.
There was a problem hiding this comment.
test_and_reset - maybe, it's a bit verbose though. Note that wait() also resets. I did update to comments on both. FWIW, a test() without resetting the signal state is also a bad idea for all the reasons I described above.
The sleep is probably quite excessive. Feel free to reduce it.
Yes, I gave it a quick test. Seemed to do the trick.
|
@amaiorano - what do you want to do with this PR? Do you want me to adopt it, close it? |
|
Sorry for not responding earlier. I suppose we could close this. I think it would be nice to have a sample that's a bit closer to how a debugger would actually work. |
2 participants
Make it so that the debugger is actually looping through all lines while
in running, so that pause stops on whatever current line it's on, or
setting a breakpoint will eventually hit.
Also fixed breakpoint verified state for the last line, as line numbers
are 1-based.