Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(58)

Issue 136123012: Fix problems with absolute value function. (Closed)

Created:
6 years, 10 months ago by ramant (doing other things)
Modified:
6 years, 10 months ago
Reviewers:
Ryan Hamilton
CC:
chromium-reviews, cbentzel+watch_chromium.org
Visibility:
Public.

Description

Fix problems with absolute value function. The standard library provides seven absolute value functions.  From C, there is abs, labs, llabs, fabs, fabsf, and fabsl for the types int, long, long long, double, float and long double.  Due to numeric conversions, these functions can accept arguments of any numeric types, silently converting between types.  C++ libraries has std::abs, which provides overloads for all these types, making it a better choice.  The following problems have been observed, along with their fixes: 1) The wrong sized absolute value was used.  Using abs for a long long would cause a cast to int first, resulting in unexpected values outside of the range (INT_MIN, INT_MAX).  Switch to using std::abs 2) Using the wrong type of absolute value function.  Passing a floating point number to an integer absolute value will cause rounding of values, especially important for values close to zero.  Switch to using std::abs 3) Taking the absolute value of an unsigned value.  This usually has no effect, but may have some effects from casting.  Fixed by removing the absolute value function. 4) Attempting to find the absolute difference between two unsigned values.  These were transformed from abs(a - b) to (a > b ? a - b : b - a).  This ternary expression is usually stored in another variable. 5) Accidentally capturing the conditional such as abs(a - b < threshold).  Moving the parentheses over to fix. 6) Some static_casts were used to, such as from uint32 -> int64, to get a signed value in the absolute value. 7) A few types were changed from unsigned to signed. Merge internal change: 61013787 R=rch@chromium.org

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -3 lines) Patch
M net/quic/congestion_control/inter_arrival_overuse_detector.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/quic/congestion_control/tcp_cubic_sender.cc View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 2 (0 generated)
ramant (doing other things)
6 years, 10 months ago (2014-02-08 00:15:27 UTC) #1
Ryan Hamilton
6 years, 10 months ago (2014-02-08 01:32:20 UTC) #2
lgtm

Powered by Google App Engine
This is Rietveld 408576698