From c6fafd3e90b05e02dd313e93e3d4455f59e46d4d Mon Sep 17 00:00:00 2001 From: Daniel Bertalan Date: Mon, 5 Jul 2021 18:38:17 +0200 Subject: [PATCH] AK+Userland: Add generic `AK::abs()` function and use it Previously, in LibGFX's `Point` class, calculated distances were passed to the integer `abs` function, even if the stored type was a float. This caused the value to unexpectedly be truncated. Luckily, this API was not used with floating point types, but that can change in the future, so why not fix it now :^) Since we are in C++, we can use function overloading to make things easy, and to automatically use the right version. This is even better than the LibC/LibM functions, as using a bit of hackery, they are able to be constant-evaluated. They use compiler intrinsics, so they do not depend on external code and the compiler can emit the most optimized code by default. Since we aren't using the C++ standard library's trick of importing everything into the `AK` namespace, this `abs` function cannot be exported to the global namespace, as the names would clash. --- AK/StdLibExtras.h | 20 +++++++++++++++++++ Userland/Demos/Eyes/EyesWidget.cpp | 5 +++-- Userland/Libraries/LibGfx/Point.h | 5 ++--- Userland/Libraries/LibJS/Runtime/Value.cpp | 4 ++-- .../LibWeb/Layout/FlexFormattingContext.cpp | 4 ++-- 5 files changed, 29 insertions(+), 9 deletions(-) diff --git a/AK/StdLibExtras.h b/AK/StdLibExtras.h index ac074bf72a..30a23b65bd 100644 --- a/AK/StdLibExtras.h +++ b/AK/StdLibExtras.h @@ -125,6 +125,26 @@ constexpr bool is_constant_evaluated() #endif } +// These can't be exported into the global namespace as they would clash with the C standard library. + +#define __DEFINE_GENERIC_ABS(type, zero, intrinsic) \ + constexpr type abs(type num) \ + { \ + if (is_constant_evaluated()) \ + return num < zero ? -num : num; \ + else \ + return __builtin_##intrinsic(num); \ + } + +__DEFINE_GENERIC_ABS(int, 0, abs); +__DEFINE_GENERIC_ABS(long, 0l, labs); +__DEFINE_GENERIC_ABS(long long, 0ll, llabs); +#ifndef KERNEL +__DEFINE_GENERIC_ABS(float, 0.0f, fabsf); +__DEFINE_GENERIC_ABS(double, 0.0, fabs); +__DEFINE_GENERIC_ABS(long double, 0.0l, fabsl); +#endif + } using AK::array_size; diff --git a/Userland/Demos/Eyes/EyesWidget.cpp b/Userland/Demos/Eyes/EyesWidget.cpp index 2fe07ba04d..87eb099787 100644 --- a/Userland/Demos/Eyes/EyesWidget.cpp +++ b/Userland/Demos/Eyes/EyesWidget.cpp @@ -5,6 +5,7 @@ */ #include "EyesWidget.h" +#include #include #include #include @@ -89,14 +90,14 @@ Gfx::IntPoint EyesWidget::pupil_center(Gfx::IntRect& eyeball_bounds) const double max_distance_along_this_direction; // clang-format off - if (dx != 0 && abs(dx) >= abs(dy)) { + if (dx != 0 && AK::abs(dx) >= AK::abs(dy)) { double slope = dy / dx; double slope_squared = slope * slope; max_distance_along_this_direction = 0.25 * sqrt( (slope_squared + 1) / (1 / width_squared + slope_squared / height_squared) ); - } else if (dy != 0 && abs(dy) >= abs(dx)) { + } else if (dy != 0 && AK::abs(dy) >= AK::abs(dx)) { double slope = dx / dy; double slope_squared = slope * slope; max_distance_along_this_direction = 0.25 * sqrt( diff --git a/Userland/Libraries/LibGfx/Point.h b/Userland/Libraries/LibGfx/Point.h index c5a3c54be4..1d1f8f2df9 100644 --- a/Userland/Libraries/LibGfx/Point.h +++ b/Userland/Libraries/LibGfx/Point.h @@ -13,7 +13,6 @@ #include #include #include -#include namespace Gfx { @@ -217,7 +216,7 @@ public: // Returns pixels moved from other in either direction [[nodiscard]] T pixels_moved(Point const& other) const { - return max(abs(dx_relative_to(other)), abs(dy_relative_to(other))); + return max(AK::abs(dx_relative_to(other)), AK::abs(dy_relative_to(other))); } [[nodiscard]] float distance_from(Point const& other) const @@ -229,7 +228,7 @@ public: [[nodiscard]] Point absolute_relative_distance_to(Point const& other) const { - return { abs(dx_relative_to(other)), abs(dy_relative_to(other)) }; + return { AK::abs(dx_relative_to(other)), AK::abs(dy_relative_to(other)) }; } template diff --git a/Userland/Libraries/LibJS/Runtime/Value.cpp b/Userland/Libraries/LibJS/Runtime/Value.cpp index c60a7f8157..5666b28e71 100644 --- a/Userland/Libraries/LibJS/Runtime/Value.cpp +++ b/Userland/Libraries/LibJS/Runtime/Value.cpp @@ -179,7 +179,7 @@ static String double_to_string(double d) else builder.append('-'); - builder.append(String::number(fabs(exponent - 1))); + builder.append(String::number(AK::abs(exponent - 1))); return builder.to_string(); } @@ -193,7 +193,7 @@ static String double_to_string(double d) else builder.append('-'); - builder.append(String::number(fabs(exponent - 1))); + builder.append(String::number(AK::abs(exponent - 1))); return builder.to_string(); } diff --git a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp index 9887bbf469..1c9cb5d431 100644 --- a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp +++ b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp @@ -482,7 +482,7 @@ void FlexFormattingContext::run(Box& box, LayoutMode) if (sum_of_unfrozen_flex_items_flex_factors < 1) { auto intermediate_free_space = initial_free_space * sum_of_unfrozen_flex_items_flex_factors; - if (abs(intermediate_free_space) < abs(remaining_free_space)) + if (AK::abs(intermediate_free_space) < AK::abs(remaining_free_space)) remaining_free_space = intermediate_free_space; } @@ -503,7 +503,7 @@ void FlexFormattingContext::run(Box& box, LayoutMode) for_each_unfrozen_item([&](FlexItem* flex_item) { float ratio = flex_item->scaled_flex_shrink_factor / sum_of_scaled_flex_shrink_factor_of_unfrozen_items; - flex_item->target_main_size = flex_item->flex_base_size - (abs(remaining_free_space) * ratio); + flex_item->target_main_size = flex_item->flex_base_size - (AK::abs(remaining_free_space) * ratio); }); } } else {