Use std::fabs() instead of abs() in rationalTime.cpp #721
Conversation
Codecov Report
@@ Coverage Diff @@
## master #721 +/- ##
=======================================
Coverage 81.88% 81.88%
=======================================
Files 72 72
Lines 2755 2755
=======================================
Hits 2256 2256
Misses 499 499
Continue to review full report at Codecov.
|
src/opentime/rationalTime.cpp
Outdated
| @@ -290,7 +290,7 @@ RationalTime::to_time_string() const { | |||
| // result and return the string at the end with a '-'. This provides | |||
| // compatibility with ffmpeg, which allows negative time strings. | |||
| if (std::signbit(total_seconds)) { | |||
| total_seconds = abs(total_seconds); | |||
| total_seconds = std::abs(total_seconds); | |||
There was a problem hiding this comment.
This is interesting. It makes me wonder what kinds of subtle bugs this may have introduced, because abs is definitely going to coerce to an int.
I think I would still prefer using std::fabs even if std::abs has an overload to double just to drive home the point. @meshula is there a reason we'd prefer the C vs non-C++ version? (Ie fabs instead of std::fabs)?
Should probably also add a unit test for this.
There was a problem hiding this comment.
In this local context, abs doesn't cause any bugs through integral promotion. My recommendation to correct the c standard library abs to std::abs was in error, it should be std::fabs as you say. We are idiomatically using the std:: wrapped C functions throughout otio. Under the hood, they should evaluate to the same thing.
There was a problem hiding this comment.
ok cool. So then just std::fabs change as noted. Thanks! Good catch!
KarthikRIyer
changed the title
|
@ssteinbach @reinecke The tox build failed - is that spurious, or related to this change in some way? |
|
Looks like the run failed just for python3, while trying to run coverage. I'll restart it. |
No description provided.