-
Notifications
You must be signed in to change notification settings - Fork 358
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
Fix SLI sleep function (fixes #973) #976
Conversation
remove duplicity of implementation for int and double arg.
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.
Looks mostly good, but see my comment.
sli/slicontrol.cc
Outdated
{ | ||
throw BadParameterValue( String::compose( | ||
"t < %1s required.", std::numeric_limits< int >::max() ) ); |
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 it be t <=
?
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.
The limit is around 68 years (for 32-bit int), so I think it does not really matter much and avoids any potential corner cases. Therefore, I chose the simpler <
.
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.
Alright 👍
@espenhgn Could you check if this works for you? |
Hi,
|
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 very much like this simplification of the code and only have a small comment on the structure.
sli/slicontrol.cc
Outdated
"t < %1s required.", std::numeric_limits< int >::max() ) ); | ||
} | ||
else if ( t > 0 ) | ||
{ |
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 this cascade of checks would be nicer and more readable if there were
- stand-alone
if
s for the checks, whichthrow
in case of a parameter error (i.e. noelse if
s) - no
else if
block at the end, but the code from that just behind the checks, thus rather having simpler code than treating the case oft == 0
differently fromt > 0
. The overhead is probably negligible.
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.
@jougs Thank you for your feedback! I have now tidied the code as you suggested.
sli/slicontrol.cc
Outdated
|
||
// on some systems, select may modify tv, therefore, it cannot be const | ||
struct timeval tv = { t_sec, t_musec }; | ||
select( 0, 0, 0, 0, &tv ); |
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.
On second thought, I find it a bit weird to use select
here instead of just
#include <unistd.h>
...
usleep(microseconds);
Can you please change (or at least comment on) this? Thanks!
@jougs I now replaced |
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 guess this is the best we can do for this function. Many thanks :-)
Revise implementation of sleep to work correctly with 32-bit signed int; remove duplicity of implementation for int and double arg. This eliminates a warning about narrowing, which is an error in strict C++11.
Also, it seemed that sleep times given as doubles were not handled correctly in the past, e.g. 2.75 s.
This fixes #973.