Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[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

Merged
merged 1 commit into from
Jun 19, 2025
Merged

Conversation

philnik777
Copy link
Contributor

@philnik777 philnik777 commented May 12, 2025

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>.

@philnik777 philnik777 force-pushed the move_abs branch 3 times, most recently from 3104e4c to f742590 Compare May 15, 2025 16:20
Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

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}}

@philnik777 philnik777 marked this pull request as ready for review June 10, 2025 12:39
@philnik777 philnik777 requested a review from a team as a code owner June 10, 2025 12:39
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jun 10, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 10, 2025

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This avoids including &lt;stdlib.h&gt; in &lt;math.h&gt;.


Full diff: https://github.com/llvm/llvm-project/pull/139586.diff

4 Files Affected:

  • (modified) libcxx/include/__math/abs.h (+24)
  • (modified) libcxx/include/math.h (+1-10)
  • (modified) libcxx/include/stdlib.h (+2-17)
  • (modified) libcxx/test/std/numerics/c.math/abs.verify.cpp (+2-2)
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}}

Copy link
Member

@ldionne ldionne left a 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.

libcxx/include/__math/abs.h Show resolved Hide resolved
@philnik777 philnik777 merged commit 584cc37 into llvm:main Jun 19, 2025
388 of 406 checks passed
@philnik777 philnik777 deleted the move_abs branch June 19, 2025 08:37
copybara-service bot pushed a commit to google-ai-edge/LiteRT that referenced this pull request Jun 25, 2025
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
copybara-service bot pushed a commit to google-ai-edge/LiteRT that referenced this pull request Jun 25, 2025
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
@DKLoehr
Copy link
Contributor

DKLoehr commented Jun 26, 2025

As a heads up, after this PR we found that some of the template/overload resolution for abs was different. In tensorflow, previously we had std::abs<float>. After this change, we had to change it to [](float f) { return std::abs(f); }, or use a static_cast to the right function type.

For more context, see https://crbug.com/427716648. Totally fixable, but it took a little while to figure out the fix.

aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 26, 2025
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}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.