Rostime overflow bugs #61
Conversation
| @@ -52,26 +52,30 @@ namespace ros { | |||
| template<class T> | |||
| T& DurationBase<T>::fromSec(double d) | |||
| { | |||
| sec = (int32_t)floor(d); | |||
| int64_t sec64 = (int64_t)floor(d); | |||
| if (sec64 < INT_MIN || sec64 > INT_MAX) | |||
There was a problem hiding this comment.
You might consider using INT32_MIN and INT32_MAX instead, reference:
http://en.cppreference.com/w/cpp/types/integer
(here and below)
There was a problem hiding this comment.
I agree using (U)INT32_MIN|MAX would indeed be nicer. I kept it with (U)INT_MIN|MAX for now since those are already being used within the code. I would rather not mix them and don't want to update the existing code.
| int64_t sec64 = (int64_t)floor(t); | ||
| if (sec64 < 0 || sec64 > UINT_MAX) | ||
| throw std::runtime_error("Duration is out of dual 32-bit range"); | ||
| sec = (int32_t)sec64; |
There was a problem hiding this comment.
Why cast to int32_t, not to uint32_t? For me, uint seems logical.
There was a problem hiding this comment.
Absolutely. Thank you for catching this. Too much copy-n-paste between Duration and Time. I created #63 to address it.
fixed cast introduced in #61
| sec = (uint32_t)floor(t); | ||
| int64_t sec64 = (int64_t)floor(t); | ||
| if (sec64 < 0 || sec64 > UINT_MAX) | ||
| throw std::runtime_error("Duration is out of dual 32-bit range"); |
There was a problem hiding this comment.
Consider changing this to:
"Time is out of dual unsigned 32-bit range".
Avoids ambiguity for people who are trying to debug, and it is more accurate. The ros_comm repo is currently failing the empty_timestamp_crash_check test with this exception, which is why I'm making the suggestion.
There was a problem hiding this comment.
Thank you for pointing it out. Fixed in f824e46.
Replaces #58.
The first commit uses the tests proposed in #58. The second commit updates the implementation to pass the new tests. In contrast to the proposed fix in #58 the patch is smaller and imo simpler in logic.
@ros/ros_team Please review.