-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port more C tests to Cpp #727
Conversation
… cpp-tests-missing
… cpp-tests-missing
… cpp-tests-missing
Altered from c-version because of differing support for physical actions in cpp
The naming collides with a timeout parameter in the MainGenerator of the cpp target so the test is named differently from c.
… cpp-tests-missing
it seems logically equivalent to the C version but does not terminate after 0 secs
… cpp-tests-missing
#683db3a seems to have fixed the last bug on windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I added a few minor comments, mostly regarding the use of time in C++ code bodies.
test/Cpp/src/DoublePort.lf
Outdated
reaction(in, in2) {= | ||
auto elapsed_time = get_elapsed_logical_time(); | ||
if(in.is_present()){ | ||
reactor::log::Info() << "At tag (" << elapsed_time << ", " << get_logical_time() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_logical_time()
will return a time point on the logical time axis. This is not the microstep. Unfortunately, there is currently no convenient API for getting a tag in C++ (see lf-lang/reactor-cpp#3). Currently, you would need to use environment()->logical_time().micro_step()
to get the microstep of the current tag.
test/Cpp/src/ImportComposition.lf
Outdated
auto receive_time = get_elapsed_logical_time(); | ||
reactor::log::Info() << "Received " << *imp_comp.y.get() << " at time " << receive_time; | ||
received = true; | ||
if(receive_time.count() != 55000000LL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to chrono, this can be done simpler in C++: if(receive_time != 55ms)
test/Cpp/src/Alignment.lf
Outdated
state last_invoked:{=reactor::TimePoint=}; | ||
reaction(ok, in) {= | ||
if (ok.is_present() && in.is_present()) { | ||
reactor::log::Info() << "Destination: Input " << *in.get() << " is a prime at tag ( " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here it is fine to only print a logical time (as opposed to the complete tag including the microstep). However, the text should then better say "logical time" instead of tag. Also the output becomes easier to read when using get_ellapsed_logical_time
, as it then starts counting from 0, instead of the physical start time.
test/Cpp/src/PhysicalConnection.lf
Outdated
input in:int; | ||
reaction(in) {= | ||
auto time{get_elapsed_physical_time()}; | ||
reactor::log::Info() << "Received " << *in.get() << " at logical time " << time.count() << "."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can print time directly, it will be formatted for you ;)
test/Cpp/src/PhysicalConnection.lf
Outdated
reactor Destination { | ||
input in:int; | ||
reaction(in) {= | ||
auto time{get_elapsed_physical_time()}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be logical time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I copied the wrong error text here. This is the PhysicalConnection Test so the physical time is correct I think.
test/Cpp/src/PhysicalConnection.lf
Outdated
reaction(in) {= | ||
auto time{get_elapsed_physical_time()}; | ||
reactor::log::Info() << "Received " << *in.get() << " at logical time " << time.count() << "."; | ||
if(time.count() <= 0LL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to compare with reactor::TimePoint::zero()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not find zero() in TimePoint but reactor::Duration::zero() seems to do the job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, right, that is what I meant ;)
reaction(x) {= | ||
auto elapsed_time = get_elapsed_logical_time(); | ||
reactor::log::Info() << "Result is " << *x.get(); | ||
reactor::log::Info() << "Current logical time is: " << elapsed_time.count(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, no count
is needed.
test/Cpp/src/SlowingClock.lf
Outdated
=} | ||
reaction(a) -> a {= | ||
auto elapsed_logical_time = get_elapsed_logical_time(); | ||
reactor::log::Info() << "Logical time since start: " << elapsed_logical_time.count() << " nsec."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can also remove the count() and "nsec"
reaction(in) {= | ||
auto current{get_elapsed_logical_time()}; | ||
if(current > 11ms ){ | ||
reactor::log::Error() << "Received at: " << current.count() << ". Failed to enforce timeout."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No count
needed.
… cpp-tests-missing
currently not implemented in Cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks!
The TimeoutZero test is referenced in #725 and currently I would remove it from the PR if the discussion does not resolve the issue.
The Timeout test (without the Zero) had to be renamed to Timeout_Test because of a naming collision.
I hope I retained the original meaning of the tests. Maybe people who worked on those could comment if they spot any glaring issues, thought maybe @Soroosh129 for example?