Katherine: Implement sleep, timeouts, and concept of timescale#3
Katherine: Implement sleep, timeouts, and concept of timescale#3UnknownEclipse wants to merge 8 commits intomainfrom
Conversation
UnknownEclipse
left a comment
UnknownEclipse
left a comment
There was a problem hiding this comment.
Looks great! Have a few comments (and need review from the others) but looks good!
There was a problem hiding this comment.
We may need to do some conflict resolution on the CMake when merging these PR's. It seems like you're adding gtest and the tests executable right? Anything else that should be handled?
There was a problem hiding this comment.
Yeah gtest is the only one but it was mostly so I could test my code, I could remove it if it's causing issues
| void sleep(std::chrono::milliseconds sim_ms); | ||
|
|
||
| // Get the current time in simulation time | ||
| std::chrono::time_point<std::chrono::system_clock> clock_gettime(); |
There was a problem hiding this comment.
clock_gettime can work with multiple clock id's. We should log the ones that get used by the simulation target rather than using the same for everything in the future (no changes right now though)
There was a problem hiding this comment.
Ok let me know when/whether I should make changes for this
MatthewMattei
left a comment
MatthewMattei
left a comment
There was a problem hiding this comment.
I second Ben's comments; my only addition is that it would be nice to add a one-line comment or so for each test explaining what they're intended to cover so that it's easy to understand test coverage at a glance
CutieOwl
left a comment
CutieOwl
left a comment
There was a problem hiding this comment.
Thanks for the comments guys! Let me know if there's anything else I need to do before the end of the class :)
| void sleep(std::chrono::milliseconds sim_ms); | ||
|
|
||
| // Get the current time in simulation time | ||
| std::chrono::time_point<std::chrono::system_clock> clock_gettime(); |
There was a problem hiding this comment.
Ok let me know when/whether I should make changes for this
3 participants
No description provided.