Add PRESUBMIT check to cc to ensure that C++ std::abs is used

This is try #2, previous patch r214144 was reverted in r214149.

Before this patch, it is possible to use abs() without the std::
namespace qualifier, which may link to the C standard library
implementation of abs(). Thus, someone using abs(float) may get
wrong results because C standard version will convert the float
to an int. This patch updates the occurrences of of abs() and
fabs() in cc/ (though technically none were incorrect, thankfully)
and adds a PRESUBMIT to enforce that all uses of abs from now on
have an explicit std:: to resolve them correctly.

BUG=261900
R=enne@chromium.org

Review URL: https://codereview.chromium.org/21061004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@214225 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 53 insertions, 10 deletions

diff --git a/cc/PRESUBMIT.py b/cc/PRESUBMIT.py
index 83c22ab..bcf71ef 100644
--- a/cc/PRESUBMIT.py
+++ b/cc/PRESUBMIT.py
@@ -57,6 +57,46 @@ def CheckAsserts(input_api, output_api, white_list=CC_SOURCE_FILES, black_list=N
       items=notreached_files)]
   return []
 
+def CheckStdAbs(input_api, output_api,
+                white_list=CC_SOURCE_FILES, black_list=None):
+  black_list = tuple(black_list or input_api.DEFAULT_BLACK_LIST)
+  source_file_filter = lambda x: input_api.FilterSourceFile(x,
+                                                            white_list,
+                                                            black_list)
+
+  using_std_abs_files = []
+  found_fabs_files = []
+  missing_std_prefix_files = []
+
+  for f in input_api.AffectedSourceFiles(source_file_filter):
+    contents = input_api.ReadFile(f, 'rb')
+    if re.search(r"using std::f?abs;", contents):
+      using_std_abs_files.append(f.LocalPath())
+    if re.search(r"\bfabsf?\(", contents):
+      found_fabs_files.append(f.LocalPath());
+    # The following regular expression in words says:
+    # "if there is no 'std::' behind an 'abs(' or 'absf(',
+    # or if there is no 'std::' behind a 'fabs(' or 'fabsf(',
+    # then it's a match."
+    if re.search(r"((?<!std::)(\babsf?\()|(?<!std::)(\bfabsf?\())", contents):
+      missing_std_prefix_files.append(f.LocalPath())
+
+  result = []
+  if using_std_abs_files:
+    result.append(output_api.PresubmitError(
+        'These files have "using std::abs" which is not permitted.',
+        items=using_std_abs_files))
+  if found_fabs_files:
+    result.append(output_api.PresubmitError(
+        'std::abs() should be used instead of std::fabs() for consistency.',
+        items=found_fabs_files))
+  if missing_std_prefix_files:
+    result.append(output_api.PresubmitError(
+        'These files use abs(), absf(), fabs(), or fabsf() without qualifying '
+        'the std namespace. Please use std::abs() in all places.',
+        items=missing_std_prefix_files))
+  return result
+
 def CheckSpamLogging(input_api,
                      output_api,
                      white_list=CC_SOURCE_FILES,
@@ -140,6 +180,7 @@ def CheckTodos(input_api, output_api):
 def CheckChangeOnUpload(input_api, output_api):
   results = []
   results += CheckAsserts(input_api, output_api)
+  results += CheckStdAbs(input_api, output_api)
   results += CheckSpamLogging(input_api, output_api, black_list=CC_PERF_TEST)
   results += CheckPassByValue(input_api, output_api)
   results += CheckChangeLintsClean(input_api, output_api)
diff --git a/cc/animation/timing_function.cc b/cc/animation/timing_function.cc
index f5a4f9f..65d607f 100644
--- a/cc/animation/timing_function.cc
+++ b/cc/animation/timing_function.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include <algorithm>
+#include <cmath>
 
 #include "base/logging.h"
 #include "cc/animation/timing_function.h"
@@ -47,14 +48,14 @@ static double bezier_interp(double x1,
   double step = 1.0;
   for (int i = 0; i < MAX_STEPS; ++i, step *= 0.5) {
     const double error = eval_bezier(x1, x2, t) - x;
-    if (fabs(error) < BEZIER_EPSILON)
+    if (std::abs(error) < BEZIER_EPSILON)
       break;
     t += error > 0.0 ? -step : step;
   }
 
   // We should have terminated the above loop because we got close to x, not
   // because we exceeded MAX_STEPS. Do a DCHECK here to confirm.
-  DCHECK_GT(BEZIER_EPSILON, fabs(eval_bezier(x1, x2, t) - x));
+  DCHECK_GT(BEZIER_EPSILON, std::abs(eval_bezier(x1, x2, t) - x));
 
   // Step 2. Return the interpolated y values at the t we computed above.
   return eval_bezier(y1, y2, t);
diff --git a/cc/animation/transform_operation.cc b/cc/animation/transform_operation.cc
index 854901d..dacea06 100644
--- a/cc/animation/transform_operation.cc
+++ b/cc/animation/transform_operation.cc
@@ -60,7 +60,7 @@ static bool ShareSameAxis(const TransformOperation* from,
   double dot = to->rotate.axis.x * from->rotate.axis.x +
                to->rotate.axis.y * from->rotate.axis.y +
                to->rotate.axis.z * from->rotate.axis.z;
-  double error = std::fabs(1.0 - (dot * dot) / (length_2 * other_length_2));
+  double error = std::abs(1.0 - (dot * dot) / (length_2 * other_length_2));
   bool result = error < kAngleEpsilon;
   if (result) {
     *axis_x = to->rotate.axis.x;
diff --git a/cc/debug/overdraw_metrics.cc b/cc/debug/overdraw_metrics.cc
index 0b3e838..5b19370 100644
--- a/cc/debug/overdraw_metrics.cc
+++ b/cc/debug/overdraw_metrics.cc
@@ -39,7 +39,7 @@ static inline float PolygonArea(gfx::PointF points[8], int num_points) {
   float area = 0;
   for (int i = 0; i < num_points; ++i)
     area += WedgeProduct(points[i], points[(i+1)%num_points]);
-  return fabs(0.5f * area);
+  return std::abs(0.5f * area);
 }
 
 // Takes a given quad, maps it by the given transformation, and gives the area
diff --git a/cc/layers/tiled_layer.cc b/cc/layers/tiled_layer.cc
index 2aa36a0..753cdfa 100644
--- a/cc/layers/tiled_layer.cc
+++ b/cc/layers/tiled_layer.cc
@@ -802,10 +802,10 @@ bool TiledLayer::Update(ResourceUpdateQueue* queue,
   delta = gfx::Vector2d(delta.x() == 0 ? 1 : delta.x(),
                         delta.y() == 0 ? 1 : delta.y());
   gfx::Vector2d major_delta =
-      (abs(delta.x()) > abs(delta.y())) ? gfx::Vector2d(delta.x(), 0)
+      (std::abs(delta.x()) > std::abs(delta.y())) ? gfx::Vector2d(delta.x(), 0)
                                         : gfx::Vector2d(0, delta.y());
   gfx::Vector2d minor_delta =
-      (abs(delta.x()) <= abs(delta.y())) ? gfx::Vector2d(delta.x(), 0)
+      (std::abs(delta.x()) <= std::abs(delta.y())) ? gfx::Vector2d(delta.x(), 0)
                                          : gfx::Vector2d(0, delta.y());
   gfx::Vector2d deltas[4] = { major_delta, minor_delta, -major_delta,
                               -minor_delta };
diff --git a/cc/trees/layer_sorter.cc b/cc/trees/layer_sorter.cc
index 6729b19..73fe3ef 100644
--- a/cc/trees/layer_sorter.cc
+++ b/cc/trees/layer_sorter.cc
@@ -159,7 +159,8 @@ LayerSorter::ABCompareResult LayerSorter::CheckOverlap(LayerShape* a,
     return ABeforeB;
 
   float max_diff =
-      fabsf(max_positive) > fabsf(max_negative) ? max_positive : max_negative;
+      std::abs(max_positive) > std::abs(max_negative) ?
+          max_positive : max_negative;
 
   // If the results are inconsistent (and the z difference substantial to rule
   // out numerical errors) then the layers are intersecting. We will still
@@ -169,7 +170,7 @@ LayerSorter::ABCompareResult LayerSorter::CheckOverlap(LayerShape* a,
   if (max_positive > z_threshold && max_negative < -z_threshold)
     *weight = 0.f;
   else
-    *weight = fabsf(max_diff);
+    *weight = std::abs(max_diff);
 
   // Maintain relative order if the layers have the same depth at all
   // intersection points.
@@ -293,7 +294,7 @@ void LayerSorter::CreateGraphNodes(LayerImplList::iterator first,
     min_z = std::min(min_z, node.shape.transform_origin.z());
   }
 
-  z_range_ = fabsf(max_z - min_z);
+  z_range_ = std::abs(max_z - min_z);
 }
 
 void LayerSorter::CreateGraphEdges() {
diff --git a/cc/trees/layer_tree_host_perftest.cc b/cc/trees/layer_tree_host_perftest.cc
index 4103158..fc7673c 100644
--- a/cc/trees/layer_tree_host_perftest.cc
+++ b/cc/trees/layer_tree_host_perftest.cc
@@ -268,7 +268,7 @@ class PageScaleImplSidePaintingPerfTest : public ImplSidePaintingPerfTest {
                                     : time_in_two_intervals;
     // Normalize time to -1..1.
     float normalized = 2.f * time_in_one_interval - 1.f;
-    float scale_factor = std::fabs(normalized) * (max_scale_ - 1.f) + 1.f;
+    float scale_factor = std::abs(normalized) * (max_scale_ - 1.f) + 1.f;
     float total_scale = normalized < 0.f ? 1.f / scale_factor : scale_factor;
 
     float desired_delta =