From 3193742f0f9014ac342f54b0085986cef724252d Mon Sep 17 00:00:00 2001 From: danakj Date: Fri, 5 Jun 2015 11:15:10 -0700 Subject: [PATCH] Replace gfx::ClampToInt with base::saturated_cast. This found a bug in saturated_cast that was covered by ClampToInt tests so it fixes that. If you have a floating point value of MAX_INT, then comparing an integer against it for equality with promote it to a float and it will compare true. However if you cast the float to an int, its actually outside the bounds of integer, so you can end up with a negative int as a result. Added unittests to check this for saturated_cast. Review URL: https://codereview.chromium.org/1164063005 Cr-Commit-Position: refs/heads/master@{#333092} --- base/numerics/safe_conversions_impl.h | 14 +++++------ base/numerics/safe_numerics_unittest.cc | 4 +++ .../views/extensions/bookmark_app_bubble_view.cc | 4 +-- ui/gfx/canvas_skia.cc | 7 +++--- ui/gfx/geometry/rect_unittest.cc | 10 -------- ui/gfx/geometry/safe_integer_conversions.h | 23 ++++++----------- .../geometry/safe_integer_conversions_unittest.cc | 29 ---------------------- ui/gfx/transform_unittest.cc | 11 -------- 8 files changed, 24 insertions(+), 78 deletions(-) diff --git a/base/numerics/safe_conversions_impl.h b/base/numerics/safe_conversions_impl.h index 504ce7ef2977..41570671601b 100644 --- a/base/numerics/safe_conversions_impl.h +++ b/base/numerics/safe_conversions_impl.h @@ -148,10 +148,10 @@ struct DstRangeRelationToSrcRangeImpl { static RangeConstraint Check(Src value) { return std::numeric_limits::is_iec559 - ? GetRangeConstraint(value <= std::numeric_limits::max(), - value >= -std::numeric_limits::max()) - : GetRangeConstraint(value <= std::numeric_limits::max(), - value >= std::numeric_limits::min()); + ? GetRangeConstraint((value < std::numeric_limits::max()), + (value > -std::numeric_limits::max())) + : GetRangeConstraint((value < std::numeric_limits::max()), + (value > std::numeric_limits::min())); } }; @@ -163,7 +163,7 @@ struct DstRangeRelationToSrcRangeImpl { static RangeConstraint Check(Src value) { - return GetRangeConstraint(value <= std::numeric_limits::max(), true); + return GetRangeConstraint(value < std::numeric_limits::max(), true); } }; @@ -178,7 +178,7 @@ struct DstRangeRelationToSrcRangeImpl sizeof(Src) ? RANGE_VALID : GetRangeConstraint( - value <= static_cast(std::numeric_limits::max()), + value < static_cast(std::numeric_limits::max()), true); } }; @@ -195,7 +195,7 @@ struct DstRangeRelationToSrcRangeImpl::value >= MaxExponent::value) ? GetRangeConstraint(true, value >= static_cast(0)) : GetRangeConstraint( - value <= static_cast(std::numeric_limits::max()), + value < static_cast(std::numeric_limits::max()), value >= static_cast(0)); } }; diff --git a/base/numerics/safe_numerics_unittest.cc b/base/numerics/safe_numerics_unittest.cc index b8098c40a8fe..6f9a966c0156 100644 --- a/base/numerics/safe_numerics_unittest.cc +++ b/base/numerics/safe_numerics_unittest.cc @@ -562,6 +562,8 @@ TEST(SafeNumerics, CastTests) { double double_small = 1.0; double double_large = numeric_limits::max(); double double_infinity = numeric_limits::infinity(); + double double_large_int = numeric_limits::max(); + double double_small_int = numeric_limits::min(); // Just test that the casts compile, since the other tests cover logic. EXPECT_EQ(0, checked_cast(static_cast(0))); @@ -594,5 +596,7 @@ TEST(SafeNumerics, CastTests) { EXPECT_EQ(saturated_cast(double_large), numeric_limits::max()); EXPECT_EQ(saturated_cast(double_large), double_infinity); EXPECT_EQ(saturated_cast(-double_large), -double_infinity); + EXPECT_EQ(numeric_limits::min(), saturated_cast(double_small_int)); + EXPECT_EQ(numeric_limits::max(), saturated_cast(double_large_int)); } diff --git a/chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc b/chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc index 46f8d15f3b2a..0b3fede7c23e 100644 --- a/chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc +++ b/chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc @@ -4,6 +4,7 @@ #include "chrome/browser/ui/views/extensions/bookmark_app_bubble_view.h" +#include "base/numerics/safe_conversions.h" #include "base/strings/string16.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" @@ -15,7 +16,6 @@ #include "ui/base/l10n/l10n_util.h" #include "ui/base/resource/resource_bundle.h" #include "ui/events/keycodes/keyboard_codes.h" -#include "ui/gfx/geometry/safe_integer_conversions.h" #include "ui/gfx/image/image_skia.h" #include "ui/gfx/image/image_skia_source.h" #include "ui/views/controls/button/checkbox.h" @@ -47,7 +47,7 @@ class WebAppInfoImageSource : public gfx::ImageSkiaSource { private: gfx::ImageSkiaRep GetImageForScale(float scale) override { - int size = gfx::ClampToInt(dip_size_ * scale); + int size = base::saturated_cast(dip_size_ * scale); for (const auto& icon_info : info_.icons) { if (icon_info.width == size) { return gfx::ImageSkiaRep(icon_info.data, scale); diff --git a/ui/gfx/canvas_skia.cc b/ui/gfx/canvas_skia.cc index 428c32a8b590..0d1b70d0d300 100644 --- a/ui/gfx/canvas_skia.cc +++ b/ui/gfx/canvas_skia.cc @@ -7,10 +7,10 @@ #include "base/i18n/rtl.h" #include "base/logging.h" #include "base/memory/scoped_ptr.h" +#include "base/numerics/safe_conversions.h" #include "ui/gfx/font_list.h" #include "ui/gfx/geometry/insets.h" #include "ui/gfx/geometry/rect.h" -#include "ui/gfx/geometry/safe_integer_conversions.h" #include "ui/gfx/range/range.h" #include "ui/gfx/render_text.h" #include "ui/gfx/shadow_value.h" @@ -141,7 +141,7 @@ void Canvas::SizeStringFloat(const base::string16& text, std::vector strings; ElideRectangleText(text, font_list, *width, INT_MAX, wrap_behavior, &strings); - Rect rect(ClampToInt(*width), INT_MAX); + Rect rect(base::saturated_cast(*width), INT_MAX); scoped_ptr render_text(RenderText::CreateInstance()); UpdateRenderText(rect, base::string16(), font_list, flags, 0, render_text.get()); @@ -161,7 +161,8 @@ void Canvas::SizeStringFloat(const base::string16& text, *height = h; } else { scoped_ptr render_text(RenderText::CreateInstance()); - Rect rect(ClampToInt(*width), ClampToInt(*height)); + Rect rect(base::saturated_cast(*width), + base::saturated_cast(*height)); base::string16 adjusted_text = text; StripAcceleratorChars(flags, &adjusted_text); UpdateRenderText(rect, adjusted_text, font_list, flags, 0, diff --git a/ui/gfx/geometry/rect_unittest.cc b/ui/gfx/geometry/rect_unittest.cc index 8c8370b6c6ea..00a9677f0410 100644 --- a/ui/gfx/geometry/rect_unittest.cc +++ b/ui/gfx/geometry/rect_unittest.cc @@ -497,11 +497,6 @@ TEST(RectTest, ToEnclosedRect) { std::numeric_limits::max() }, { 20000.5f, 20000.5f, 0.5f, 0.5f, 20001, 20001, 0, 0 }, - { std::numeric_limits::quiet_NaN(), - std::numeric_limits::quiet_NaN(), - std::numeric_limits::quiet_NaN(), - std::numeric_limits::quiet_NaN(), - 0, 0, 0, 0 } }; for (size_t i = 0; i < arraysize(tests); ++i) { @@ -549,11 +544,6 @@ TEST(RectTest, ToEnclosingRect) { std::numeric_limits::max() }, { 20000.5f, 20000.5f, 0.5f, 0.5f, 20000, 20000, 1, 1 }, - { std::numeric_limits::quiet_NaN(), - std::numeric_limits::quiet_NaN(), - std::numeric_limits::quiet_NaN(), - std::numeric_limits::quiet_NaN(), - 0, 0, 0, 0 } }; for (size_t i = 0; i < arraysize(tests); ++i) { diff --git a/ui/gfx/geometry/safe_integer_conversions.h b/ui/gfx/geometry/safe_integer_conversions.h index 873378018b49..5efe134f0793 100644 --- a/ui/gfx/geometry/safe_integer_conversions.h +++ b/ui/gfx/geometry/safe_integer_conversions.h @@ -8,34 +8,25 @@ #include #include +#include "base/numerics/safe_conversions.h" #include "ui/gfx/gfx_export.h" namespace gfx { -inline int ClampToInt(float value) { - if (value != value) - return 0; // no int NaN. - if (value >= std::numeric_limits::max()) - return std::numeric_limits::max(); - if (value <= std::numeric_limits::min()) - return std::numeric_limits::min(); - return static_cast(value); -} - inline int ToFlooredInt(float value) { - return ClampToInt(std::floor(value)); + return base::saturated_cast(std::floor(value)); } inline int ToCeiledInt(float value) { - return ClampToInt(std::ceil(value)); + return base::saturated_cast(std::ceil(value)); } inline int ToFlooredInt(double value) { - return ClampToInt(std::floor(value)); + return base::saturated_cast(std::floor(value)); } inline int ToCeiledInt(double value) { - return ClampToInt(std::ceil(value)); + return base::saturated_cast(std::ceil(value)); } inline int ToRoundedInt(float value) { @@ -44,7 +35,7 @@ inline int ToRoundedInt(float value) { rounded = std::floor(value + 0.5f); else rounded = std::ceil(value - 0.5f); - return ClampToInt(rounded); + return base::saturated_cast(rounded); } inline int ToRoundedInt(double value) { @@ -53,7 +44,7 @@ inline int ToRoundedInt(double value) { rounded = std::floor(value + 0.5); else rounded = std::ceil(value - 0.5); - return ClampToInt(rounded); + return base::saturated_cast(rounded); } inline bool IsExpressibleAsInt(float value) { diff --git a/ui/gfx/geometry/safe_integer_conversions_unittest.cc b/ui/gfx/geometry/safe_integer_conversions_unittest.cc index 099dc4d7df56..91bdbb8d3593 100644 --- a/ui/gfx/geometry/safe_integer_conversions_unittest.cc +++ b/ui/gfx/geometry/safe_integer_conversions_unittest.cc @@ -10,32 +10,7 @@ namespace gfx { -TEST(SafeIntegerConversions, ClampToInt) { - EXPECT_EQ(0, ClampToInt(std::numeric_limits::quiet_NaN())); - - float max = std::numeric_limits::max(); - float min = std::numeric_limits::min(); - float infinity = std::numeric_limits::infinity(); - - int int_max = std::numeric_limits::max(); - int int_min = std::numeric_limits::min(); - - EXPECT_EQ(int_max, ClampToInt(infinity)); - EXPECT_EQ(int_max, ClampToInt(max)); - EXPECT_EQ(int_max, ClampToInt(max + 100)); - - EXPECT_EQ(-100, ClampToInt(-100.5f)); - EXPECT_EQ(0, ClampToInt(0)); - EXPECT_EQ(100, ClampToInt(100.5f)); - - EXPECT_EQ(int_min, ClampToInt(-infinity)); - EXPECT_EQ(int_min, ClampToInt(min)); - EXPECT_EQ(int_min, ClampToInt(min - 100)); -} - TEST(SafeIntegerConversions, ToFlooredInt) { - EXPECT_EQ(0, ToFlooredInt(std::numeric_limits::quiet_NaN())); - float max = std::numeric_limits::max(); float min = std::numeric_limits::min(); float infinity = std::numeric_limits::infinity(); @@ -57,8 +32,6 @@ TEST(SafeIntegerConversions, ToFlooredInt) { } TEST(SafeIntegerConversions, ToCeiledInt) { - EXPECT_EQ(0, ToCeiledInt(std::numeric_limits::quiet_NaN())); - float max = std::numeric_limits::max(); float min = std::numeric_limits::min(); float infinity = std::numeric_limits::infinity(); @@ -80,8 +53,6 @@ TEST(SafeIntegerConversions, ToCeiledInt) { } TEST(SafeIntegerConversions, ToRoundedInt) { - EXPECT_EQ(0, ToRoundedInt(std::numeric_limits::quiet_NaN())); - float max = std::numeric_limits::max(); float min = std::numeric_limits::min(); float infinity = std::numeric_limits::infinity(); diff --git a/ui/gfx/transform_unittest.cc b/ui/gfx/transform_unittest.cc index 19441425e4b8..eb0b2fdcc699 100644 --- a/ui/gfx/transform_unittest.cc +++ b/ui/gfx/transform_unittest.cc @@ -448,10 +448,6 @@ TEST(XFormTest, ConcatTranslate2D) { { 0, 0, 10.0f, 20.0f, 10, 20}, { 0, 0, -10.0f, -20.0f, 0, 0}, { 0, 0, -10.0f, -20.0f, -10, -20}, - { 0, 0, - std::numeric_limits::quiet_NaN(), - std::numeric_limits::quiet_NaN(), - 10, 20}, }; Transform xform; @@ -481,7 +477,6 @@ TEST(XFormTest, ConcatScale2D) { { 1, .1f, 1}, { 1, 100.0f, 100}, { 1, -1.0f, -100}, - { 1, std::numeric_limits::quiet_NaN(), 1} }; Transform xform; @@ -513,7 +508,6 @@ TEST(XFormTest, ConcatRotate2D) { { 1, 0, 90.0f, 0, 1}, { 1, 0, 360.0f, 0, 1}, { 1, 0, 0.0f, 0, 1}, - { 1, 0, std::numeric_limits::quiet_NaN(), 1, 0} }; Transform xform; @@ -541,10 +535,6 @@ TEST(XFormTest, SetTranslate2D) { { 0, 0, 10.0f, 20.0f, 10, 20}, { 10, 20, 10.0f, 20.0f, 20, 40}, { 10, 20, 0.0f, 0.0f, 10, 20}, - { 0, 0, - std::numeric_limits::quiet_NaN(), - std::numeric_limits::quiet_NaN(), - 0, 0} }; for (size_t i = 0; i < arraysize(test_cases); ++i) { @@ -597,7 +587,6 @@ TEST(XFormTest, SetScale2D) { { 1, 1.0f, 1}, { 1, 0.0f, 0}, { 0, 10.0f, 0}, - { 1, std::numeric_limits::quiet_NaN(), 0}, }; for (size_t i = 0; i < arraysize(test_cases); ++i) { -- 2.11.4.GIT