-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[libc++] Move std::abs into __math/abs.h #139586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3104e4c
to
f742590
Compare
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- libcxx/include/__math/abs.h libcxx/include/math.h libcxx/include/stdlib.h libcxx/test/std/numerics/c.math/abs.verify.cpp View the diff from clang-format here.diff --git a/libcxx/test/std/numerics/c.math/abs.verify.cpp b/libcxx/test/std/numerics/c.math/abs.verify.cpp
index cc30112f0..9e5bfda52 100644
--- a/libcxx/test/std/numerics/c.math/abs.verify.cpp
+++ b/libcxx/test/std/numerics/c.math/abs.verify.cpp
@@ -13,10 +13,12 @@ void f() {
(void)std::abs(ui); // expected-error {{call to 'abs' is ambiguous}}
unsigned char uc = -5;
- (void)std::abs(uc); // expected-warning 0-1 {{taking the absolute value of unsigned type 'unsigned char' has no effect}}
+ (void)std::abs(
+ uc); // expected-warning 0-1 {{taking the absolute value of unsigned type 'unsigned char' has no effect}}
unsigned short us = -5;
- (void)std::abs(us); // expected-warning 0-1 {{taking the absolute value of unsigned type 'unsigned short' has no effect}}
+ (void)std::abs(
+ us); // expected-warning 0-1 {{taking the absolute value of unsigned type 'unsigned short' has no effect}}
unsigned long ul = -5;
(void)std::abs(ul); // expected-error {{call to 'abs' is ambiguous}}
|
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesThis avoids including Full diff: https://github.com/llvm/llvm-project/pull/139586.diff 4 Files Affected:
diff --git a/libcxx/include/__math/abs.h b/libcxx/include/__math/abs.h
index fc3bf3a2c7c32..b780159f11ebf 100644
--- a/libcxx/include/__math/abs.h
+++ b/libcxx/include/__math/abs.h
@@ -39,6 +39,30 @@ template <class _A1, __enable_if_t<is_integral<_A1>::value, int> = 0>
return __builtin_fabs((double)__x);
}
+// abs
+
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI inline float abs(float __x) _NOEXCEPT { return __builtin_fabsf(__x); }
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI inline double abs(double __x) _NOEXCEPT { return __builtin_fabs(__x); }
+
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI inline long double abs(long double __x) _NOEXCEPT {
+ return __builtin_fabsl(__x);
+}
+
+template <class = int>
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI inline int abs(int __x) _NOEXCEPT {
+ return __builtin_abs(__x);
+}
+
+template <class = int>
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI inline long abs(long __x) _NOEXCEPT {
+ return __builtin_labs(__x);
+}
+
+template <class = int>
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI inline long long abs(long long __x) _NOEXCEPT {
+ return __builtin_llabs(__x);
+}
+
} // namespace __math
_LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/include/math.h b/libcxx/include/math.h
index de2dacde282c1..929bef6385043 100644
--- a/libcxx/include/math.h
+++ b/libcxx/include/math.h
@@ -378,9 +378,7 @@ extern "C++" {
# include <__math/traits.h>
# include <__math/trigonometric_functions.h>
# include <__type_traits/enable_if.h>
-# include <__type_traits/is_floating_point.h>
# include <__type_traits/is_integral.h>
-# include <stdlib.h>
// fpclassify relies on implementation-defined constants, so we can't move it to a detail header
_LIBCPP_BEGIN_NAMESPACE_STD
@@ -431,19 +429,12 @@ using std::__math::isnormal;
using std::__math::isunordered;
# endif // _LIBCPP_MSVCRT
-// abs
-//
-// handled in stdlib.h
-
-// div
-//
-// handled in stdlib.h
-
// We have to provide double overloads for <math.h> to work on platforms that don't provide the full set of math
// functions. To make the overload set work with multiple functions that take the same arguments, we make our overloads
// templates. Functions are preferred over function templates during overload resolution, which means that our overload
// will only be selected when the C library doesn't provide one.
+using std::__math::abs;
using std::__math::acos;
using std::__math::acosh;
using std::__math::asin;
diff --git a/libcxx/include/stdlib.h b/libcxx/include/stdlib.h
index 39550f36bb6ed..8dfdfa416f088 100644
--- a/libcxx/include/stdlib.h
+++ b/libcxx/include/stdlib.h
@@ -106,23 +106,8 @@ extern "C++" {
# undef llabs
# endif
-// MSVCRT already has the correct prototype in <stdlib.h> if __cplusplus is defined
-# if !defined(_LIBCPP_MSVCRT)
-[[__nodiscard__]] inline _LIBCPP_HIDE_FROM_ABI long abs(long __x) _NOEXCEPT { return __builtin_labs(__x); }
-[[__nodiscard__]] inline _LIBCPP_HIDE_FROM_ABI long long abs(long long __x) _NOEXCEPT { return __builtin_llabs(__x); }
-# endif // !defined(_LIBCPP_MSVCRT)
-
-[[__nodiscard__]] inline _LIBCPP_HIDE_FROM_ABI float abs(float __lcpp_x) _NOEXCEPT {
- return __builtin_fabsf(__lcpp_x); // Use builtins to prevent needing math.h
-}
-
-[[__nodiscard__]] inline _LIBCPP_HIDE_FROM_ABI double abs(double __lcpp_x) _NOEXCEPT {
- return __builtin_fabs(__lcpp_x);
-}
-
-[[__nodiscard__]] inline _LIBCPP_HIDE_FROM_ABI long double abs(long double __lcpp_x) _NOEXCEPT {
- return __builtin_fabsl(__lcpp_x);
-}
+# include <__math/abs.h>
+using std::__math::abs;
// div
diff --git a/libcxx/test/std/numerics/c.math/abs.verify.cpp b/libcxx/test/std/numerics/c.math/abs.verify.cpp
index dec80762b214f..cc30112f0c312 100644
--- a/libcxx/test/std/numerics/c.math/abs.verify.cpp
+++ b/libcxx/test/std/numerics/c.math/abs.verify.cpp
@@ -13,10 +13,10 @@ void f() {
(void)std::abs(ui); // expected-error {{call to 'abs' is ambiguous}}
unsigned char uc = -5;
- (void)std::abs(uc); // expected-warning {{taking the absolute value of unsigned type 'unsigned char' has no effect}}
+ (void)std::abs(uc); // expected-warning 0-1 {{taking the absolute value of unsigned type 'unsigned char' has no effect}}
unsigned short us = -5;
- (void)std::abs(us); // expected-warning {{taking the absolute value of unsigned type 'unsigned short' has no effect}}
+ (void)std::abs(us); // expected-warning 0-1 {{taking the absolute value of unsigned type 'unsigned short' has no effect}}
unsigned long ul = -5;
(void)std::abs(ul); // expected-error {{call to 'abs' is ambiguous}}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup. I think this makes sense but I have a few questions. Also, this could break users since we're removing a transitive include of <stdlib.h>
from <math.h>
. That's not a reason not to do it, but pinging @llvm/libcxx-vendors for awareness.
Imported from GitHub PR tensorflow/tensorflow#95959 This is required to build with the most recent version of LLVM's libc++, which moved the `abs` function from `stdlib.h` to `math.h` in llvm/llvm-project#139586. Copybara import of the project: -- 2fa2ab36dae8afc9cf838a5ce1726e4ba48afec6 by Devon Loehr <dloehr@google.com>: Include math.h in elementwise.cc Merging this change closes #95959 FUTURE_COPYBARA_INTEGRATE_REVIEW=tensorflow/tensorflow#95959 from DKLoehr:math 2fa2ab36dae8afc9cf838a5ce1726e4ba48afec6 PiperOrigin-RevId: 775719122
Imported from GitHub PR tensorflow/tensorflow#95959 This is required to build with the most recent version of LLVM's libc++, which moved the `abs` function from `stdlib.h` to `math.h` in llvm/llvm-project#139586. Copybara import of the project: -- 2fa2ab36dae8afc9cf838a5ce1726e4ba48afec6 by Devon Loehr <dloehr@google.com>: Include math.h in elementwise.cc Merging this change closes #95959 FUTURE_COPYBARA_INTEGRATE_REVIEW=tensorflow/tensorflow#95959 from DKLoehr:math 2fa2ab36dae8afc9cf838a5ce1726e4ba48afec6 PiperOrigin-RevId: 775719122
As a heads up, after this PR we found that some of the template/overload resolution for For more context, see https://crbug.com/427716648. Totally fixable, but it took a little while to figure out the fix. |
After llvm/llvm-project#139586, this header is no longer transitively included by <cmath>, so we need to include it directly. Bug: 427774670 Change-Id: I9d892d30f410ee55d55afddf33c7aabc3be21344 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6679831 Commit-Queue: Mohannad Farrag <aymanm@google.com> Auto-Submit: Devon Loehr <dloehr@google.com> Reviewed-by: Mohannad Farrag <aymanm@google.com> Cr-Commit-Position: refs/heads/main@{#1479187}
template <class = int>
is also added to our implementations to avoid an ambiguity between the libc's version and our version when both are visible.This avoids including
<stdlib.h>
in<math.h>
.